<div dir="ltr">Hi James,<div><br></div><div>I consider this problem an InstCombine deficiency.</div><div>Consider the following patch:</div><div><div>diff --git a/lib/Transforms/InstCombine/InstCombineCompares.cpp b/lib/Transforms/InstCombine/InstCombineCompares.cpp</div><div>index 223bba0..4827686 100644</div><div>--- a/lib/Transforms/InstCombine/InstCombineCompares.cpp</div><div>+++ b/lib/Transforms/InstCombine/InstCombineCompares.cpp</div><div>@@ -3970,6 +3970,19 @@ Instruction *InstCombiner::visitFCmpInst(FCmpInst &I) {</div><div> }</div><div> }</div><div> </div><div>+ // Test if the FCmpInst instruction is used exclusively by a select as</div><div>+ // part of a minimum or maximum operation. If so, refrain from doing</div><div>+ // any other folding. This helps out other analyses which understand</div><div>+ // non-obfuscated minimum and maximum idioms, such as ScalarEvolution</div><div>+ // and CodeGen. And in this case, at least one of the comparison</div><div>+ // operands has at least one user besides the compare (the select),</div><div>+ // which would often largely negate the benefit of folding anyway.</div><div>+ if (I.hasOneUse())</div><div>+ if (SelectInst *SI = dyn_cast<SelectInst>(*I.user_begin()))</div><div>+ if ((SI->getOperand(1) == Op0 && SI->getOperand(2) == Op1) ||</div><div>+ (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1))</div><div>+ return nullptr;</div><div>+</div><div> // Handle fcmp with constant RHS</div><div> if (Constant *RHSC = dyn_cast<Constant>(Op1)) {</div><div> if (Instruction *LHSI = dyn_cast<Instruction>(Op0))</div><div>diff --git a/lib/Transforms/InstCombine/InstructionCombining.cpp b/lib/Transforms/InstCombine/InstructionCombining.cpp</div><div>index f62941f..6ebc8db 100644</div><div>--- a/lib/Transforms/InstCombine/InstructionCombining.cpp</div><div>+++ b/lib/Transforms/InstCombine/InstructionCombining.cpp</div><div>@@ -714,6 +714,22 @@ Instruction *InstCombiner::FoldOpIntoSelect(Instruction &Op, SelectInst *SI) {</div><div> return nullptr;</div><div> }</div><div> </div><div>+ // Test if a CmpInst instruction is used exclusively by a select as</div><div>+ // part of a minimum or maximum operation. If so, refrain from doing</div><div>+ // any other folding. This helps out other analyses which understand</div><div>+ // non-obfuscated minimum and maximum idioms, such as ScalarEvolution</div><div>+ // and CodeGen. And in this case, at least one of the comparison</div><div>+ // operands has at least one user besides the compare (the select),</div><div>+ // which would often largely negate the benefit of folding anyway.</div><div>+ if (auto *CI = dyn_cast<CmpInst>(SI->getCondition())) {</div><div>+ if (CI->hasOneUse()) {</div><div>+ Value *Op0 = CI->getOperand(0), *Op1 = CI->getOperand(1);</div><div>+ if ((SI->getOperand(1) == Op0 && SI->getOperand(2) == Op1) ||</div><div>+ (SI->getOperand(2) == Op0 && SI->getOperand(1) == Op1))</div><div>+ return nullptr;</div><div>+ }</div><div>+ }</div><div>+</div><div> Value *SelectTrueVal = FoldOperationIntoSelectOperand(Op, TV, this);</div><div> Value *SelectFalseVal = FoldOperationIntoSelectOperand(Op, FV, this);</div></div><div><br></div><div>With this patch in mind, running it on:</div><div><div>define i32 @f(float %x) {</div><div> %1 = fcmp olt float %x, 0.000000e+00</div><div> %2 = select i1 %1, float 0.000000e+00, float %x</div><div> %3 = fptoui float %2 to i32</div><div> ret i32 %3</div><div>}</div></div><div><br></div><div>results in no change.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 26, 2015 at 3:49 PM, James Molloy <span dir="ltr"><<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Philip,<br><br><div>Thanks very much for reviewing my proposal. I should say that I generally agree with your points, and am still in multiple minds about what the best approach would look like.</div><span class=""><div><br></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">> I don't see the challenge here. Matching a compare+select as a min/max for the purpose of the cost model under a target specific hook seems quite straightforward. What am I missing?</span><br></div></span><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">> ...</span></div><span class=""><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">> Er, not sure I get your point here. Not having to match two distinct families of representation is an advantage, not a disadvantage. The branch form should be getting converted into the select form much earlier in the optimizer. Which cases are you worried about here?</span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div></span><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">The awkward part is twofold. Firstly, the example I gave in my previous message where InstCombine mangles the pattern by pushing an fptoui between the icmp and the select. There's two approaches to this I see - first, match early and deal with the pattern as an intrinsic so it can't be broken up. Second, have a (flexible, deals with edge cases) way of matching at any stage in the pipeline - this might be ripping some of the matching logic out of InstCombine and exposing it as a utility. This might be a lose in compile time though.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Secondly branches. You're right that we speculate branches to selects, but only to a small threshold. I upped the threshold so at least we'll get "min+min" or "max+min" to be speculated, but larger sequences such as "min+min+min" (folding four-input minimum) will still end up with a branch.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">I've been doing experiments with how much of the compiler I need to touch when inserting these intrinsics, and yes I do agree with you - random testing shows the compiler does need quite a bit of teaching, in InstCombine at least, to not regress randomly generated simple testcases. So I'm starting to come around more to your way of thinking.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">The only thing I haven't solved in my mind yet if we don't go down the intrinsics route is d</span><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">ealing with branches - we can't just keep upping the speculation threshold. Perhaps identify these (and only these) and up the speculation threshold in these cases only?</span></div><div><br></div><div>Matching late in CGP would solve the sharing code across backends problem, so after your feedback I'm leaning further towards this approach.</div><div><br></div><div>Do you have any further comments?</div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Sun, 26 Apr 2015 at 20:04 Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<div>On 04/23/2015 07:42 AM, James Molloy
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>I've just started again on trying to solve the problem of
getting decent code generation for min, max and abs idioms as
used by the programmer and as emitted by the loop vectorizer.</div>
<div><br>
</div>
<div>I've been looking at doing this as a target DAGCombine, but
actually I think:</div>
<div> 1. it's incredibly complex to do at that stage and it
limits all the work I do to just one target.</div>
<div> 2. It's also much more difficult to test.</div>
<div> 3. The loop and SLP vectorizers still don't have a cost
model for them - they're just seen as compare+selects.</div>
</div>
</blockquote></div><div bgcolor="#FFFFFF" text="#000000">
I don't see the challenge here. Matching a compare+select as a
min/max for the purpose of the cost model under a target specific
hook seems quite straightforward. What am I missing?</div><div bgcolor="#FFFFFF" text="#000000"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>So my proposal is:</div>
<div> * To add new intrinsics for minimum, maximum and absolute
value. These would have signed int/unsigned int/float variants
and be valid across all numeric types.</div>
<div> * To add a pass fairly early in the pipeline to idiom
recognize and create intrinsics. This would be controllable
per-backend - if a backend doesn't have efficient lowering for
these operations, perhaps it's best not to do the idiom
recognition.</div>
</div>
</blockquote></div><div bgcolor="#FFFFFF" text="#000000">
I am strongly opposed to this part of the proposal. I have no
problem* adding such intrinsics and matching them late (i.e.
CodeGenPrep), but I am deeply concerned about the negative impacts
of matching early. Unless you are volunteering to add support for
these intrinsics to *every* pass, I believe doing this is a
non-starter. As a good example, consider what happened recently
with the x.with.overflow intrinsics where we were missing important
simplifications on induction variable dependent checks due to early
canonicalization to a form that the rest of the optimizer didn't
understand. <br>
<br>
More generally, I'm not even sure matching these early would be the
right answer even if you were volunteering to update the entire
optimizer. Being able to fold the condition (CSE, etc..)
independently of the select and then being able to exploit a
dominating branch is extremely powerful at eliminating the min/max
operation entirely. I would be deeply concerned about giving up
that power without an incredible compelling reason. <br>
<br>
* By "no problem", I really mean that I have no opinion here. I am
neither endorsing nor opposing. <br></div><div bgcolor="#FFFFFF" text="#000000">
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>The cost model would then fall out in the wash, because we
already have a cost model for intrinsics, it would be as
simple as adding new instructions. </div>
</div>
</blockquote>
<br>
<blockquote type="cite">
<div dir="ltr">
<div>Because we idiom recognize at the IR stage instead of the
SDAG stage, we also wouldn't have to rely on the min/max
idioms being in canonical "select" form; we could match a
branch sequence also.</div>
</div>
</blockquote></div><div bgcolor="#FFFFFF" text="#000000">
Er, not sure I get your point here. Not having to match two
distinct families of representation is an advantage, not a
disadvantage. The branch form should be getting converted into the
select form much earlier in the optimizer. Which cases are you
worried about here?<br>
<blockquote type="cite"></blockquote></div><div bgcolor="#FFFFFF" text="#000000"><blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>What do you think? Is this an acceptable proposal?</div>
<div><br>
</div>
<div>Cheers,</div>
<div><br>
</div>
<div>James</div>
</div>
<br>
<fieldset></fieldset>
<br>
</blockquote></div><div bgcolor="#FFFFFF" text="#000000"><blockquote type="cite"><pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote></div></blockquote></div>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>