llvm dev - Feb 2019 - funnel shift, select, and poison

> Transforms/InstCombine/select.ll
> ===============================> define i1 @trueval_is_true(i1 %C, i1
%X) {
>   %R = select i1 %C, i1 1, i1 %X
>   ret i1 %R
> }
> =>
> define i1 @trueval_is_true(i1 %C, i1 %X) {
>   %R = or i1 %C, %X
>   ret i1 %R
> }
> ERROR: Target is more poisonous than source (when %C = #x1 & %X =
poison)
> 
> (there are many variations of these select->arithmetic transformations)
This particular little family of transformations can be reliably done by 
all of the backends I looked at, so disabling them at the IR level 
should be OK. See a bit more discussion here:
> https://bugs.llvm.org/show_bug.cgi?id=40768
John

I don't object to deferring the optimization, but let me check my poison
understanding...
  select i1 %cond, i1 true, i1 %x --> or i1 %cond, %x

1. 'select' is a poison-blocking operation, but 'or' is
non-poison-blocking, so we are propagating a potentially poisonous %x with
this transform.
2. We will fix the problem in IR by removing this transform in IR
3. The backend (SDAG) has that same transform.
4. SDAG has poison because we propagate the wrapping/exact flags to DAG
nodes.
5. Are we just sweeping the bug under the rug? Nobody cares because SDAG is
undocumented, so anything goes down there?


On Tue, Feb 26, 2019 at 2:06 PM John Regehr via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> > Transforms/InstCombine/select.ll
> > ===============================> > define i1 @trueval_is_true(i1
%C, i1 %X) {
> >   %R = select i1 %C, i1 1, i1 %X
> >   ret i1 %R
> > }
> > =>
> > define i1 @trueval_is_true(i1 %C, i1 %X) {
> >   %R = or i1 %C, %X
> >   ret i1 %R
> > }
> > ERROR: Target is more poisonous than source (when %C = #x1 & %X =
poison)
> >
> > (there are many variations of these select->arithmetic
transformations)
>
> This particular little family of transformations can be reliably done by
> all of the backends I looked at, so disabling them at the IR level
> should be OK. See a bit more discussion here:
>
> > https://bugs.llvm.org/show_bug.cgi?id=40768
>
> John
> _______________________________________________
> 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/20190227/3331249c/attachment.html>

You are right: select in SDAG has to be poison-blocking as well,  
otherwise the current lowering from IR's select to SDAG's select would  
be wrong. Which makes the select->or transformation incorrect at SDAG  
level as well.
I guess until recently people believed that poison in SDAG wasn't much  
of a problem (myself included). I was convinced otherwise with the  
test cases that Juneyoung found a few weeks ago:  
https://bugs.llvm.org/show_bug.cgi?id=40657

MI doesn't have poison AFAIK, so at least we are safe there, I hope  
(not sure about undef, though).

I don't know if there was a thorough discussion on the performance  
benefits of having poison in SDAG, though. Is it that important? are  
there loop-related optimizations that require nsw information?
If so, then we should pay the price and fix SDAG. (and extend Alive to  
verify SDAG as well -- ok, I'm hanging myself here, but..). If not,  
maybe getting rid of poison in SDAG could be a good thing.
There's also undef, which I don't know exactly what's the semantics,
but AFAIR is as bad as the IR's.

Nuno


Quoting Sanjay Patel via llvm-dev <llvm-dev at lists.llvm.org>:
> I don't object to deferring the optimization, but let me check my
poison
> understanding...
>   select i1 %cond, i1 true, i1 %x --> or i1 %cond, %x
>
> 1. 'select' is a poison-blocking operation, but 'or' is
> non-poison-blocking, so we are propagating a potentially poisonous %x with
> this transform.
> 2. We will fix the problem in IR by removing this transform in IR
> 3. The backend (SDAG) has that same transform.
> 4. SDAG has poison because we propagate the wrapping/exact flags to DAG
> nodes.
> 5. Are we just sweeping the bug under the rug? Nobody cares because SDAG is
> undocumented, so anything goes down there?
>
>
> On Tue, Feb 26, 2019 at 2:06 PM John Regehr via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> > Transforms/InstCombine/select.ll
>> > ===============================>> > define i1
@trueval_is_true(i1 %C, i1 %X) {
>> >   %R = select i1 %C, i1 1, i1 %X
>> >   ret i1 %R
>> > }
>> > =>
>> > define i1 @trueval_is_true(i1 %C, i1 %X) {
>> >   %R = or i1 %C, %X
>> >   ret i1 %R
>> > }
>> > ERROR: Target is more poisonous than source (when %C = #x1 &
%X = poison)
>> >
>> > (there are many variations of these select->arithmetic
transformations)
>>
>> This particular little family of transformations can be reliably done
by
>> all of the backends I looked at, so disabling them at the IR level
>> should be OK. See a bit more discussion here:
>>
>> > https://bugs.llvm.org/show_bug.cgi?id=40768
>>
>> John