<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 7/1/21 10:09 AM, Nikita Popov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Jul 1, 2021 at 6:44
PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</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">
<div>
<p>When working on a couple of recent changes in SCEV, I
stumbled on a couple of gaps around how we optimize the
@llvm.umin intrinsics. If I remember correctly, we
added these because they needed different poison
propagation semantics than select, and that memory
triggered a thought. <br>
</p>
</div>
</blockquote>
<div>The min/max intrinsics have the same poison semantics as
their select forms. It's the undef semantics that differ.</div>
</div>
</div>
</blockquote>
Can you add wording about poison to the LangRef for those? At the
moment, it's largely unspecified. <br>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>However, I believe that the primary motivation for having
the intrinsics is that maintaining canonical SPF form is
hard. Especially many InstCombine optimizations need to be
very careful about not breaking canonical SPF form, because
doing so will likely result in an infinite combine loop.<br>
</div>
</div>
</div>
</blockquote>
I've disagreed with this line of argument from the beginning, but
fine. I'm not going to bother getting back into that.<br>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<p> </p>
<p>I want to raise the question of whether we should
support both possible poison propagation semantics for
the select instruction. As a brief review, the two
options are:</p>
<ol>
<li>Poison propagates only through the selected
operand. e.g. "select i1 true, i32 0, i32 poison" is
not poison. <br>
</li>
<li>Poison propagates if any operand is poison. e.g.
"select i1 true, i32 0, i32 poison" is poison. </li>
</ol>
<p>Each of the two options has advantages. This was
discussed in depth a while ago, and we'd picked (1). I
don't want to reopen that discussion; I want to raise
the question of whether we should pick "both". <br>
</p>
<p>If we were to add a flag "npo" (for "no poison
operand", bikeshedding welcome!) to the select
instruction, we could represent both options. This has
a couple of advantages:</p>
<ul>
<li>We can lower all of the umin/etc.. intrinsics to
selects and avoid having to duplicate folds. This
would reduce the combinatoric space for pattern
matching optimizations. This would probably help
optimization results in practice.<br>
</li>
</ul>
</div>
</blockquote>
<div>min/max intrinsics currently don't have full support in
InstCombine yet, because we haven't ported all transforms
from SPF min/max. Once that is done and we canonicalize to
the intrinsic form, we can drop the duplicate SPF folds. </div>
</div>
</div>
</blockquote>
And how long has that work been in flight? Also, JFYI, the
transform I hit was not a SPF specific fold. It was a generic fold
on selects which had to be manually duplicated for the new umin
form. <br>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>From an optimization quality perspective, we're not ready
to canonicalize to intrinsics yet. Unfortunately,
SCEVExpander has already been switched to use intrinsics
before this work completed, which is presumably how you got
here. <br>
</div>
</div>
</div>
</blockquote>
<p>It is. Which means your preceding argument doesn't really apply
unless you want to revert the SCEVExpander change. <br>
</p>
<p>I do want to avoid tying the idea and this discussion too closely
the umin intrinsic handling. It's what sparked the thought, but
it might be worth doing even if we leave umin intrinsic as the
canonical form form for that operation. <br>
</p>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li> <br>
</li>
<li>We can restore many of the removed
select->arithmetic folds (only for the npo
selects).</li>
</ul>
</div>
</blockquote>
<div>Which transforms do you have in mind here? The select
-> and/or transform is the only one I'm aware of, but in
that case we're using select specifically for it's current
poison semantics, to prevent that fold from happening where
it is not legal.<br>
</div>
</div>
</div>
</blockquote>
<p>"Not legal" given the semantics we chose (e.g. 1 above). That's
a key point.</p>
<p>I'd have to ask Juneyoung, but I'd had the impression he had to
disable a couple others as well. <br>
</p>
<p>Also, even if not, "just" the and/or transform seems useful
enough to possibly justify the IR extension if we could infer npo
frequently. (Which, admittedly, is an unknown.)<br>
</p>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li>We can infer npo flag on a select in many cases.
(e.g. "select i1 %c, i32 0, i32 57" can be trivially
converted to "select npo i1 %c, i32 0, i32 57") This
also allows us to factor logic into testable pieces
whereas current transforms which are npo dependent
must include the no-poison inference.<br>
</li>
</ul>
<p>The major downside is that transformation code would
have to be careful to only apply transforms dependent on
"no poison operand" if the flag is set.</p>
<p>Anyways, I don't have time to actually work on this, so
I'm mostly throwing out the idea in case anyone with
time and motivation finds it interesting to pursue.</p>
</div>
</blockquote>
<div>With the above notes in mind, I'm not sure where having
the flag would really be advantageous.<br>
</div>
</div>
</div>
</blockquote>
That's why we debate these things. :)<br>
<blockquote type="cite"
cite="mid:CAF+90c_V=uc3Hm0Btab1ihzwENVsg=g6NE6yj2+m3=AOEqX=2Q@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Regards,</div>
<div>Nikita<br>
</div>
</div>
</div>
</blockquote>
</body>
</html>