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 :slight_smile: 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())) {

...

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 :slight_smile: 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

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 :slight_smile: 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.

Sounds good, thanks Duncan. If you want to handle general integer elements, then getElementAsInteger will have to start retuning an APInt. Not the end of the world I guess.

-Chris