[llvm-commits] [llvm] r155468 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/2012-04-23-Neon-Intrinsics.ll

[Lang Hames]

Hi Chandler,

+    Instruction *Arg0W =
>> +      Zext ? CastInst::CreateZExtOrBitCast(Arg0, II->getType(), "", II) :
>> +             CastInst::CreateSExtOrBitCast(Arg0, II->getType(), "", II);
>> +    Value* Arg0WS = SimplifyInstruction(Arg0W);
>> +    if (Arg0WS == 0) // If simplification fails just pass through the
>> ext'd val.
>> +      Arg0WS = Arg0W;
>>
>
> Why are you simplifying these incrementally? SimplifyInstruction is not at
> all free, it can recurse deeply and add compile time with little benefit.
>
> 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.
>

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.


> +    Instruction *SimplifiedInst = 0;
>> +    if (Value* V = SimplifyMulInst(Arg0WS, Arg1WS, TD)) {
>>
>
> I think this is an unnecessary gymnastic just to re-use the InstSimplify
> logic.
>
> 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...
>
> 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.
>
>
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.


> The specific advantages I see are:
>
> - Don't thrash the use lists by adding no-op instructions
> - Don't allocate, insert, use, then delete instructions in a common case
> (no simplification).
> - Able to extend the analysis in other ways that require forming new
> instructions.
>

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.

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.

In short - I agree this is ugly, but it seems like the least worst
tradeoff. Jumping on IRC for further discussion...

Cheers,
Lang.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120424/c7d3c386/attachment.html>