<div class="gmail_extra">Hi Chandler,</div><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+    Instruction *Arg0W =<br>
+      Zext ? CastInst::CreateZExtOrBitCast(Arg0, II->getType(), "", II) :<br>
+             CastInst::CreateSExtOrBitCast(Arg0, II->getType(), "", II);<br>
+    Value* Arg0WS = SimplifyInstruction(Arg0W);<br>
+    if (Arg0WS == 0) // If simplification fails just pass through the ext'd val.<br>
+      Arg0WS = Arg0W;<br></blockquote><div><br></div></div><div>Why are you simplifying these incrementally? SimplifyInstruction is not at all free, it can recurse deeply and add compile time with little benefit.</div><div>
<br>
</div><div>I would just check if they are the right size, and if not, extend them. We'll simplify everything if there are good ways to do so after-the-fact.</div></div></div></blockquote><div> </div><div>I'm not sure what you mean by "the right size" here? the vmull_n_* intrinsic parameters are always half the width of its result, so they'll never be "the right size" by that measure. I'll try deferring the simplification though - that might be a win.</div>
<div>  </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

+    Instruction *SimplifiedInst = 0;<br>
+    if (Value* V = SimplifyMulInst(Arg0WS, Arg1WS, TD)) {<br></blockquote><div><br></div></div><div>I think this is an unnecessary gymnastic just to re-use the InstSimplify logic.</div><div><br></div><div>InstSimplify's constraints (as you pointed out in the review thread) are that it cannot form new instructions. That doesn't mean that the right approach in InstCombine is to form additional instructions that we think might be "cheap", and then see if InstSimplify can simplify through them. Is this a pattern already established elsewhere in InstCombine? I haven't seen it, but there is a lot of InstCombine I've not looked at...</div>

<div><br></div><div>Instead, I'd rather write the actual analysis your interested in performing on these intrinsics in InstCombine, with the knowledge (and taking the action) of extending as necessary.</div><div><br>
</div></div></div></blockquote><div><br></div><div>My understanding is that extending the operands will always be a necessary precondition for using SimplifyMulInst here, given how SimplifyMulInst is currently defined. I don't want to teach SimplifyMulInst to deal with different return/operand types, because that will add a complexity and compile-time for every client of SimplifyMulInst just to handle my corner case. I also don't want to duplicate and extend the logic behind SimplifyMulInst: there's a lot of it.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div></div>
<div>The specific advantages I see are:</div><div><br></div><div>- Don't thrash the use lists by adding no-op instructions</div><div>- Don't allocate, insert, use, then delete instructions in a common case (no simplification).</div>

<div>- Able to extend the analysis in other ways that require forming new instructions.</div></div></div></blockquote><div> </div><div>I agree with the first two points, but my initial (admittedly not-well-informed) impression is that these intrinsics are rare, and these impacts will only hit functions containing these intrinsics.</div>
<div><br></div><div>I'm not so sure about the 3rd point - As noted above, I'd have to add logic (and compile-time overhead) to a common code-path, or duplicate a non-trivial amount of logic, to handle an uncommon case.</div>
<div><br></div><div>In short - I agree this is ugly, but it seems like the least worst tradeoff. Jumping on IRC for further discussion...</div><div><br></div><div>Cheers,</div><div>Lang.</div></div></div>