<div dir="ltr"><div dir="ltr"><div>I don't think anyone at that time realized that we were poisoning SDAG by extending the flags. :)</div><div><br></div><div>So yes - and this is probably setting myself up for a suicide mission - I think we should undo that decision.</div><div><br></div><div>A transform that I remember adding to SDAG that depends on wrapping was here:</div><div><a href="https://reviews.llvm.org/D13757">https://reviews.llvm.org/D13757</a><br></div><div>...and that shows that the cleanup would have to extend to target-specific files. It also suggests the solution: deal with all poison-related transforms in IR in CGP (or a dedicated codegen IR pass) before we form the DAG, and then drop those bits.<br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 27, 2019 at 9:12 AM Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt">nunoplopes@sapo.pt</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">You are right: select in SDAG has to be poison-blocking as well,  <br>
otherwise the current lowering from IR's select to SDAG's select would  <br>
be wrong. Which makes the select->or transformation incorrect at SDAG  <br>
level as well.<br>
I guess until recently people believed that poison in SDAG wasn't much  <br>
of a problem (myself included). I was convinced otherwise with the  <br>
test cases that Juneyoung found a few weeks ago:  <br>
<a href="https://bugs.llvm.org/show_bug.cgi?id=40657" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=40657</a><br>
<br>
MI doesn't have poison AFAIK, so at least we are safe there, I hope  <br>
(not sure about undef, though).<br>
<br>
I don't know if there was a thorough discussion on the performance  <br>
benefits of having poison in SDAG, though. Is it that important? are  <br>
there loop-related optimizations that require nsw information?<br>
If so, then we should pay the price and fix SDAG. (and extend Alive to  <br>
verify SDAG as well -- ok, I'm hanging myself here, but..). If not,  <br>
maybe getting rid of poison in SDAG could be a good thing.<br>
There's also undef, which I don't know exactly what's the semantics,  <br>
but AFAIR is as bad as the IR's.<br>
<br>
Nuno<br>
<br>
<br>
Quoting Sanjay Patel via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>:<br>
<br>
> I don't object to deferring the optimization, but let me check my poison<br>
> understanding...<br>
>   select i1 %cond, i1 true, i1 %x --> or i1 %cond, %x<br>
><br>
> 1. 'select' is a poison-blocking operation, but 'or' is<br>
> non-poison-blocking, so we are propagating a potentially poisonous %x with<br>
> this transform.<br>
> 2. We will fix the problem in IR by removing this transform in IR<br>
> 3. The backend (SDAG) has that same transform.<br>
> 4. SDAG has poison because we propagate the wrapping/exact flags to DAG<br>
> nodes.<br>
> 5. Are we just sweeping the bug under the rug? Nobody cares because SDAG is<br>
> undocumented, so anything goes down there?<br>
><br>
><br>
> On Tue, Feb 26, 2019 at 2:06 PM John Regehr via llvm-dev <<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
>> > Transforms/InstCombine/select.ll<br>
>> > ================================<br>
>> > define i1 @trueval_is_true(i1 %C, i1 %X) {<br>
>> >   %R = select i1 %C, i1 1, i1 %X<br>
>> >   ret i1 %R<br>
>> > }<br>
>> > =><br>
>> > define i1 @trueval_is_true(i1 %C, i1 %X) {<br>
>> >   %R = or i1 %C, %X<br>
>> >   ret i1 %R<br>
>> > }<br>
>> > ERROR: Target is more poisonous than source (when %C = #x1 & %X = poison)<br>
>> ><br>
>> > (there are many variations of these select->arithmetic transformations)<br>
>><br>
>> This particular little family of transformations can be reliably done by<br>
>> all of the backends I looked at, so disabling them at the IR level<br>
>> should be OK. See a bit more discussion here:<br>
>><br>
>> > <a href="https://bugs.llvm.org/show_bug.cgi?id=40768" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=40768</a><br>
>><br>
>> John<br>
<br>
</blockquote></div>