<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Flags isn’t the only way you can get poison in SelectionDAG; a bunch of operations produce poison. Among other things, we have shifts with an overflowing shift amount, overflowing fptosi, and AssertZext. IIRC we take advantage of the
fptosi UB to avoid redundant masking in some cases.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I guess we could go through and limit those operations to something else instead… but I’m not quite sure what that would look like. Getting rid of poison isn’t that helpful if we still have undef (particularly if we’re going to try to
get rid of undef at the IR level at some point). I guess we could get rid of undef too, but at that point I think you start seriously hurting target-independent SelectionDAG optimizations: every operation has to have a defined output for every possible input
(except operations with a chain can be UB, or produce an output that’s “defined” based on the chain). I guess that would involve adding target-defined behavior for a bunch of operations, along the lines of getBooleanContents().<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">It makes sense say poison/undef don’t exist after isel. That might block some optimizations in theory, I guess, but I think that matches the way it currently works. (We currently have “undef” operands in MachineInstrs, but that isn’t
the same thing.) Well, actually, I think the use of alias analysis in scheduling breaks this (since the result of a load which violates TBAA can be changed by the scheduler), but I don’t think that’s ever led to a practical issue.<o:p></o:p></p>
<p class="MsoNormal"><o:p></o:p></p>
<p class="MsoNormal">-Eli<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> <b>On Behalf Of
</b>Sanjay Patel via llvm-dev<br>
<b>Sent:</b> Wednesday, February 27, 2019 9:38 AM<br>
<b>To:</b> Nuno Lopes <nunoplopes@sapo.pt><br>
<b>Cc:</b> llvm-dev <llvm-dev@lists.llvm.org>; John Regehr <regehr@cs.utah.edu><br>
<b>Subject:</b> [EXT] Re: [llvm-dev] funnel shift, select, and poison<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<div>
<p class="MsoNormal">I don't think anyone at that time realized that we were poisoning SDAG by extending the flags. :)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">So yes - and this is probably setting myself up for a suicide mission - I think we should undo that decision.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">A transform that I remember adding to SDAG that depends on wrapping was here:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a href="https://reviews.llvm.org/D13757">https://reviews.llvm.org/D13757</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">...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.<o:p></o:p></p>
</div>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Wed, Feb 27, 2019 at 9:12 AM Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt">nunoplopes@sapo.pt</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt">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" 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" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=40768</a><br>
>><br>
>> John<o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>