Nuno Lopes via llvm-dev
2019-Nov-27 17:49 UTC
[llvm-dev] LangRef semantics for shufflevector with undef mask is incorrect
Ok, makes sense. My suggestion is that we patch the IR Verifier to ensure that the mask is indeed a vector of constants and/or undefs. Right now it only runs the standard checks for instructions. We will also run Alive2 on the test suite to make sure undef is never replaced in practice. Thanks, Nuno -----Original Message----- From: Eli Friedman <efriedma at quicinc.com> Sent: 27 de novembro de 2019 01:10 To: Nuno Lopes <nuno.lopes at ist.utl.pt>; LLVMdev <llvm-dev at lists.llvm.org> Cc: spatel at rotateright.com; Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; zhengyang-liu at hotmail.com; John Regehr <regehr at cs.utah.edu> Subject: RE: LangRef semantics for shufflevector with undef mask is incorrect The shuffle mask of a shufflevector is special: it's required to be a constant in a specific form. From LangRef: "The shuffle mask operand is required to be a constant vector with either constant integer or undef values." So really, we can resolve this any way we want; "undef" in this context doesn't have to mean the same thing as "undef" in other contexts. Formally, at the LangRef level, we can state that the shuffle mask is not an operand of a shufflevector; instead, it's not a value at all. It's just a description of the shuffle, defined with a grammar similar to a vector constant. Then we can talk about shuffle masks where an element is the string "undef", unrelated to the general notion of an undef value. In that context, the existing LangRef description of the result of shufflevector can be interpreted to mean exactly what it says. This would imply it's forbidden to transform the shuffle mask "<2 x i32> <i32 undef, i32 0>" to "<2 x i32> <i32 1, i32 0>" if the input might contain poison. (And this is the same logic that led to https://reviews.llvm.org/D70246 .) That said, long-term, we probably want to switch shufflevector to produce poison. -Eli> -----Original Message----- > From: Nuno Lopes <nuno.lopes at ist.utl.pt> > Sent: Tuesday, November 26, 2019 3:20 PM > To: LLVMdev <llvm-dev at lists.llvm.org> > Cc: spatel at rotateright.com; Eli Friedman <efriedma at quicinc.com>; > Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; zhengyang-liu at hotmail.com; > John Regehr <regehr at cs.utah.edu> > Subject: [EXT] LangRef semantics for shufflevector with undef mask is > incorrect > > Hi, > > This is a follow up on a discussion around shufflevector with undef > mask in > https://reviews.llvm.org/D70641 and > https://bugs.llvm.org/show_bug.cgi?id=43958. > > The current semantics of shufflevector in > http://llvm.org/docs/LangRef.html#shufflevector-instruction states: > "If the shuffle mask is undef, the result vector is undef. If any > element of the mask operand is undef, that element of the result isundef."> > We found this semantics to be problematic. TL;DR: instructions can't > detect if an operand is undef. > Long story: > Undef can be replaced with any value at any time. It's valid to > replace undef with 0 or 1 or anything else. > > A possible sequence of optimizations with sufflevector could be asfollows:> %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > undef, > i32 0> > -> > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > 2, i32 > 0> > -> > %v = <undef, %x[0]> > > So this respects the semantics in LangRef: the mask is undef, so the > resulting element is undef. > > However, there's an alternative sequence of optimizations: > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > undef, > i32 0> > -> > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > 1, i32 > 0> > -> > %v = <%x[1], %x[0]> > > So now it depends on what the value of %x[1] is. If it's poison, weobtain:> %v = <poison, %x[0]> > > This result contradicts the semantics in LangRef, even though no > individual transformation we did above is wrong. > In summary, an instruction cannot detect undef and have special > semantics for it. > > AFAICT, the only way to fix the semantics of shufflevector is to say > that if the mask is out-of-bounds, we yield poison. That's correct > because there's nothing worse than poison. > Since we can replace undef with an OOB index, an undef mask can safely > yield poison. Or it would yield one of the input elements, which is > poison in the worst case. So we get poison in both cases. > > I guess the issue to make this semantics a reality is that we would > need to introduce a poison value (which is a good thing IMHO). > Otherwise we can't continue doing some of the folds we have today > since we don't have a poison constant to replace undef when folding. > > Any comments/suggestions appreciated! > > Thanks, > Nuno
Craig Topper via llvm-dev
2019-Nov-27 18:47 UTC
[llvm-dev] LangRef semantics for shufflevector with undef mask is incorrect
I believe ShuffleVectorInst::isValidOperands checks for the mask already. It's called by Verifier::visitShuffleVectorInst. It's also called by the LL Parser and the an assert in the shuffle vector constructor. ~Craig On Wed, Nov 27, 2019 at 9:49 AM Nuno Lopes via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Ok, makes sense. > My suggestion is that we patch the IR Verifier to ensure that the mask is > indeed a vector of constants and/or undefs. Right now it only runs the > standard checks for instructions. > We will also run Alive2 on the test suite to make sure undef is never > replaced in practice. > > Thanks, > Nuno > > -----Original Message----- > From: Eli Friedman <efriedma at quicinc.com> > Sent: 27 de novembro de 2019 01:10 > To: Nuno Lopes <nuno.lopes at ist.utl.pt>; LLVMdev <llvm-dev at lists.llvm.org> > Cc: spatel at rotateright.com; Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; > zhengyang-liu at hotmail.com; John Regehr <regehr at cs.utah.edu> > Subject: RE: LangRef semantics for shufflevector with undef mask is > incorrect > > The shuffle mask of a shufflevector is special: it's required to be a > constant in a specific form. From LangRef: "The shuffle mask operand is > required to be a constant vector with either constant integer or undef > values." So really, we can resolve this any way we want; "undef" in this > context doesn't have to mean the same thing as "undef" in other contexts. > Formally, at the LangRef level, we can state that the shuffle mask is not > an > operand of a shufflevector; instead, it's not a value at all. It's just a > description of the shuffle, defined with a grammar similar to a vector > constant. Then we can talk about shuffle masks where an element is the > string "undef", unrelated to the general notion of an undef value. > > In that context, the existing LangRef description of the result of > shufflevector can be interpreted to mean exactly what it says. This would > imply it's forbidden to transform the shuffle mask "<2 x i32> <i32 undef, > i32 0>" to "<2 x i32> <i32 1, i32 0>" if the input might contain poison. > (And this is the same logic that led to https://reviews.llvm.org/D70246 .) > > That said, long-term, we probably want to switch shufflevector to produce > poison. > > -Eli > > > -----Original Message----- > > From: Nuno Lopes <nuno.lopes at ist.utl.pt> > > Sent: Tuesday, November 26, 2019 3:20 PM > > To: LLVMdev <llvm-dev at lists.llvm.org> > > Cc: spatel at rotateright.com; Eli Friedman <efriedma at quicinc.com>; > > Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; zhengyang-liu at hotmail.com; > > John Regehr <regehr at cs.utah.edu> > > Subject: [EXT] LangRef semantics for shufflevector with undef mask is > > incorrect > > > > Hi, > > > > This is a follow up on a discussion around shufflevector with undef > > mask in > > https://reviews.llvm.org/D70641 and > > https://bugs.llvm.org/show_bug.cgi?id=43958. > > > > The current semantics of shufflevector in > > http://llvm.org/docs/LangRef.html#shufflevector-instruction states: > > "If the shuffle mask is undef, the result vector is undef. If any > > element of the mask operand is undef, that element of the result is > undef." > > > > We found this semantics to be problematic. TL;DR: instructions can't > > detect if an operand is undef. > > Long story: > > Undef can be replaced with any value at any time. It's valid to > > replace undef with 0 or 1 or anything else. > > > > A possible sequence of optimizations with sufflevector could be as > follows: > > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > > undef, > > i32 0> > > -> > > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > > 2, i32 > > 0> > > -> > > %v = <undef, %x[0]> > > > > So this respects the semantics in LangRef: the mask is undef, so the > > resulting element is undef. > > > > However, there's an alternative sequence of optimizations: > > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > > undef, > > i32 0> > > -> > > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > > 1, i32 > > 0> > > -> > > %v = <%x[1], %x[0]> > > > > So now it depends on what the value of %x[1] is. If it's poison, we > obtain: > > %v = <poison, %x[0]> > > > > This result contradicts the semantics in LangRef, even though no > > individual transformation we did above is wrong. > > In summary, an instruction cannot detect undef and have special > > semantics for it. > > > > AFAICT, the only way to fix the semantics of shufflevector is to say > > that if the mask is out-of-bounds, we yield poison. That's correct > > because there's nothing worse than poison. > > Since we can replace undef with an OOB index, an undef mask can safely > > yield poison. Or it would yield one of the input elements, which is > > poison in the worst case. So we get poison in both cases. > > > > I guess the issue to make this semantics a reality is that we would > > need to introduce a poison value (which is a good thing IMHO). > > Otherwise we can't continue doing some of the folds we have today > > since we don't have a poison constant to replace undef when folding. > > > > Any comments/suggestions appreciated! > > > > Thanks, > > Nuno > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191127/3ebab305/attachment.html>
Nuno Lopes via llvm-dev
2019-Nov-27 19:47 UTC
[llvm-dev] LangRef semantics for shufflevector with undef mask is incorrect
Ah, yes, thanks. I missed that function somehow. Ok, so I think all is good then :) Thanks, Nuno -----Original Message----- From: Craig Topper Sent: Wednesday, November 27, 2019 6:47 PM Subject: Re: [llvm-dev] LangRef semantics for shufflevector with undef mask is incorrect I believe ShuffleVectorInst::isValidOperands checks for the mask already. It's called by Verifier::visitShuffleVectorInst. It's also called by the LL Parser and the an assert in the shuffle vector constructor. ~Craig On Wed, Nov 27, 2019 at 9:49 AM Nuno Lopes via llvm-dev <llvm-dev at lists.llvm.org> wrote: Ok, makes sense. My suggestion is that we patch the IR Verifier to ensure that the mask is indeed a vector of constants and/or undefs. Right now it only runs the standard checks for instructions. We will also run Alive2 on the test suite to make sure undef is never replaced in practice. Thanks, Nuno -----Original Message----- From: Eli Friedman <efriedma at quicinc.com> Sent: 27 de novembro de 2019 01:10 To: Nuno Lopes <nuno.lopes at ist.utl.pt>; LLVMdev <llvm-dev at lists.llvm.org> Cc: spatel at rotateright.com; Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; zhengyang-liu at hotmail.com; John Regehr <regehr at cs.utah.edu> Subject: RE: LangRef semantics for shufflevector with undef mask is incorrect The shuffle mask of a shufflevector is special: it's required to be a constant in a specific form. From LangRef: "The shuffle mask operand is required to be a constant vector with either constant integer or undef values." So really, we can resolve this any way we want; "undef" in this context doesn't have to mean the same thing as "undef" in other contexts. Formally, at the LangRef level, we can state that the shuffle mask is not an operand of a shufflevector; instead, it's not a value at all. It's just a description of the shuffle, defined with a grammar similar to a vector constant. Then we can talk about shuffle masks where an element is the string "undef", unrelated to the general notion of an undef value. In that context, the existing LangRef description of the result of shufflevector can be interpreted to mean exactly what it says. This would imply it's forbidden to transform the shuffle mask "<2 x i32> <i32 undef, i32 0>" to "<2 x i32> <i32 1, i32 0>" if the input might contain poison. (And this is the same logic that led to https://reviews.llvm.org/D70246 .) That said, long-term, we probably want to switch shufflevector to produce poison. -Eli> -----Original Message----- > From: Nuno Lopes <nuno.lopes at ist.utl.pt> > Sent: Tuesday, November 26, 2019 3:20 PM > To: LLVMdev <llvm-dev at lists.llvm.org> > Cc: spatel at rotateright.com; Eli Friedman <efriedma at quicinc.com>; > Juneyoung Lee <juneyoung.lee at sf.snu.ac.kr>; zhengyang-liu at hotmail.com; > John Regehr <regehr at cs.utah.edu> > Subject: [EXT] LangRef semantics for shufflevector with undef mask is > incorrect > > Hi, > > This is a follow up on a discussion around shufflevector with undef > mask in > https://reviews.llvm.org/D70641 and > https://bugs.llvm.org/show_bug.cgi?id=43958. > > The current semantics of shufflevector in > http://llvm.org/docs/LangRef.html#shufflevector-instruction states: > "If the shuffle mask is undef, the result vector is undef. If any > element of the mask operand is undef, that element of the result isundef."> > We found this semantics to be problematic. TL;DR: instructions can't > detect if an operand is undef. > Long story: > Undef can be replaced with any value at any time. It's valid to > replace undef with 0 or 1 or anything else. > > A possible sequence of optimizations with sufflevector could be asfollows:> %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > undef, > i32 0> > -> > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > 2, i32 > 0> > -> > %v = <undef, %x[0]> > > So this respects the semantics in LangRef: the mask is undef, so the > resulting element is undef. > > However, there's an alternative sequence of optimizations: > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > undef, > i32 0> > -> > %v = shufflevector <2 x float> %x, <2 x float> undef, <2 x i32> <i32 > 1, i32 > 0> > -> > %v = <%x[1], %x[0]> > > So now it depends on what the value of %x[1] is. If it's poison, weobtain:> %v = <poison, %x[0]> > > This result contradicts the semantics in LangRef, even though no > individual transformation we did above is wrong. > In summary, an instruction cannot detect undef and have special > semantics for it. > > AFAICT, the only way to fix the semantics of shufflevector is to say > that if the mask is out-of-bounds, we yield poison. That's correct > because there's nothing worse than poison. > Since we can replace undef with an OOB index, an undef mask can safely > yield poison. Or it would yield one of the input elements, which is > poison in the worst case. So we get poison in both cases. > > I guess the issue to make this semantics a reality is that we would > need to introduce a poison value (which is a good thing IMHO). > Otherwise we can't continue doing some of the folds we have today > since we don't have a poison constant to replace undef when folding. > > Any comments/suggestions appreciated! > > Thanks, > Nuno