[llvm-commits] PATCH: Teach the x86 backend to fold mask & shift patterns into an addressing mode computation

[Chandler Carruth]

On Mon, Jan 9, 2012 at 1:23 AM, Rotem, Nadav <nadav.rotem at intel.com> wrote:

> I am not familiar with the addressing mode code but I have a few small
> comments.
>

Thanks for the feedback!


> Why is this optimization disabled for vectors ?  You can use
> getScalarSizeInBits and I think that it should work.
>

I'm not familiar w/ the semantics of the masked bits computation, etc. I'm
happy to add vector support to this, but I'd like to do it in a second
patch with appropriate tests that it does the correct thing.

Return true ? :)
>

Yea, as I posted to the list, the first patch had a debug line of code in
it...


> +  SDValue X = N == Shift ? And.getOperand(0) : Shift.getOperand(0);
>
> Can you add parenthesis here ?
>

Done, I think. At least, the only precedence I find surprising is that of
the entire conditional expression, not the condition itself...


> +  // The amount of shift we're trying ot fit into the addressing mode is
> taken
>
> Typo ot -> to.
>

Fixed.


> +  SDValue NewShiftAmt = DAG.getConstant(ShiftAmt + AMShiftAmt, MVT::i8);
>
> Please use getShiftAmountTy.
>

None of the other address mode computations use it. =/ It seems like
goodness, but it requires threading a TargetLowing handle into this code.
I've got 4 or 5 more patches queued up behind this that changing the
interface in that way would make kind of annoying. I'd prefer to clean up
all of the i8 shift types in one pass afterward. That sound good to you?


I've attached an updated patch, but I made a significant change beyond your
comments. The previous patch could cause unfortunate re-computations
because we never re-computed the final value in a non-addressing-mode
context. I've changed the code to do so, and replace uses of the final
value with that computation, while leaving the addressing mode able to
select the simpler computation. Subsequent passes of ISel will trivially
fold the left-shifts into addressing modes as well when possible.

I've also disable the transform when the pre-masked shifted value has
multiple users as that will also force those users to re-compute the value.
I'm actually a bit dubious on this one, as it seems like it might still be
a win to do the transform, but I decided to start safer, and make this more
aggressive in follow-ups if benchmarks indicate it is a good tradeoff.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120109/73264d67/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crazy-addr-mode3.patch
Type: text/x-patch
Size: 10456 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120109/73264d67/attachment.bin>