llvm dev - Feb 2012 - [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())) {
...

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())) {
> ...

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())) {
>> ...
>