Duncan Sands
2012-Feb-09 21:22 UTC
[LLVMdev] Your commit 149912 "Remove some dead code and tidy things up"...
Hi Chris, this was a very tempting commit to make, unfortunately it broke pattern matching of vectors of booleans. The problem is that a ConstantDataVector is only formed if the element type is one of i8, i16, etc. So vectors of funky types, or not so funky types like i1, are no longer matched. I noticed this while working on PR11948. The good thing is that the testcase there no longer crashes the compiler because earlier patterns now fail to match :) The bad thing is that a -instsimplify testcase like the following no longer passes: define <2 x i1> @vectorselect1(<2 x i1> %cond) { ; CHECK: @vectorselect1 %invert = xor <2 x i1> %cond, <i1 1, i1 1> %s = select <2 x i1> %invert, <2 x i32> <i32 0, i32 0>, <2 x i32> <i32 1, i32 1> %c = icmp ne <2 x i32> %s, <i32 0, i32 0> ret <2 x i1> %c ; CHECK: ret <2 x i1> %cond } Ciao, Duncan.> Index: PatternMatch.h > ==================================================================> --- PatternMatch.h (revision 149911) > +++ PatternMatch.h (revision 149912) > @@ -98,13 +98,6 @@ > Res = &CI->getValue(); > return true; > } > - // FIXME: Remove this. > - if (ConstantVector *CV = dyn_cast<ConstantVector>(V)) > - if (ConstantInt *CI > - dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) { > - Res = &CI->getValue(); > - return true; > - } > if (ConstantDataVector *CV = dyn_cast<ConstantDataVector>(V)) > if (ConstantInt *CI > dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) {...
Chris Lattner
2012-Feb-10 04:43 UTC
[LLVMdev] Your commit 149912 "Remove some dead code and tidy things up"...
On Feb 9, 2012, at 1:22 PM, Duncan Sands wrote:> Hi Chris, this was a very tempting commit to make, unfortunately it broke > pattern matching of vectors of booleans. The problem is that a > ConstantDataVector is only formed if the element type is one of i8, i16, etc. > So vectors of funky types, or not so funky types like i1, are no longer > matched. I noticed this while working on PR11948. The good thing is that > the testcase there no longer crashes the compiler because earlier patterns > now fail to match :) The bad thing is that a -instsimplify testcase like the > following no longer passes: > define <2 x i1> @vectorselect1(<2 x i1> %cond) { > ; CHECK: @vectorselect1 > %invert = xor <2 x i1> %cond, <i1 1, i1 1> > %s = select <2 x i1> %invert, <2 x i32> <i32 0, i32 0>, <2 x i32> <i32 1, i32 1> > %c = icmp ne <2 x i32> %s, <i32 0, i32 0> > ret <2 x i1> %c > ; CHECK: ret <2 x i1> %cond > }Hi Duncan, I'm ok with adding this back. OTOH, it might be better to allow ConstantData to work with i1's. Would just handling i1 be enough, or do you think it's worth it to go more general? -Chris> > Ciao, Duncan. > >> Index: PatternMatch.h >> ==================================================================>> --- PatternMatch.h (revision 149911) >> +++ PatternMatch.h (revision 149912) >> @@ -98,13 +98,6 @@ >> Res = &CI->getValue(); >> return true; >> } >> - // FIXME: Remove this. >> - if (ConstantVector *CV = dyn_cast<ConstantVector>(V)) >> - if (ConstantInt *CI >> - dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) { >> - Res = &CI->getValue(); >> - return true; >> - } >> if (ConstantDataVector *CV = dyn_cast<ConstantDataVector>(V)) >> if (ConstantInt *CI >> dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) { > ...
Duncan Sands
2012-Feb-10 12:13 UTC
[LLVMdev] Your commit 149912 "Remove some dead code and tidy things up"...
Hi Chris,>> Hi Chris, this was a very tempting commit to make, unfortunately it broke >> pattern matching of vectors of booleans. The problem is that a >> ConstantDataVector is only formed if the element type is one of i8, i16, etc. >> So vectors of funky types, or not so funky types like i1, are no longer >> matched. I noticed this while working on PR11948. The good thing is that >> the testcase there no longer crashes the compiler because earlier patterns >> now fail to match :) The bad thing is that a -instsimplify testcase like the >> following no longer passes: >> define<2 x i1> @vectorselect1(<2 x i1> %cond) { >> ; CHECK: @vectorselect1 >> %invert = xor<2 x i1> %cond,<i1 1, i1 1> >> %s = select<2 x i1> %invert,<2 x i32> <i32 0, i32 0>,<2 x i32> <i32 1, i32 1> >> %c = icmp ne<2 x i32> %s,<i32 0, i32 0> >> ret<2 x i1> %c >> ; CHECK: ret<2 x i1> %cond >> } > > Hi Duncan, > > I'm ok with adding this back. OTOH, it might be better to allow ConstantData to work with i1's. Would just handling i1 be enough, or do you think it's worth it to go more general?while more general types are hardly ever used, I think it's nonetheless worth having ConstantData support them, because it makes the mental model simpler: if a constant is only made up of numbers, then it's ConstantData. I think what I'll do is revert this commit for the moment, and see if I can find a reasonable way of implementing the general case. Ciao, Duncan.> > -Chris > >> >> Ciao, Duncan. >> >>> Index: PatternMatch.h >>> ==================================================================>>> --- PatternMatch.h (revision 149911) >>> +++ PatternMatch.h (revision 149912) >>> @@ -98,13 +98,6 @@ >>> Res =&CI->getValue(); >>> return true; >>> } >>> - // FIXME: Remove this. >>> - if (ConstantVector *CV = dyn_cast<ConstantVector>(V)) >>> - if (ConstantInt *CI >>> - dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) { >>> - Res =&CI->getValue(); >>> - return true; >>> - } >>> if (ConstantDataVector *CV = dyn_cast<ConstantDataVector>(V)) >>> if (ConstantInt *CI >>> dyn_cast_or_null<ConstantInt>(CV->getSplatValue())) { >> ... >
Maybe Matching Threads
- [LLVMdev] Your commit 149912 "Remove some dead code and tidy things up"...
- [LLVMdev] Your commit 149912 "Remove some dead code and tidy things up"...
- [LLVMdev] Inverse of ConstantFP::get and similar functions?
- [LLVMdev] Inverse of ConstantFP::get and similar functions?
- [LLVMdev] Inverse of ConstantFP::get and similar functions?