[PATCH] D60789: [x86] try to widen 'shl' as part of LEA formation

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1920-1921
+
+    // All of the bits that would have been shifted out must be 0 (nuw). That
+    // way, the wider shift can't preserve any 1 bits that should get killed.
+    APInt HighZeros = APInt::getHighBitsSet(Shl.getValueSizeInBits(),
----------------
lebedev.ri wrote:
> This comment reads weird.
> It is talking about the case like: https://rise4fun.com/Alive/DQd
> Maybe something closer to
> ```
> // The new shift must be NUW, that is, it must be safe to swap zext and shl around.
> ```
Agreed - that's not very clear, but let's make sure we're seeing the same thing.
The pattern difference I was trying to highlight looks like this:
https://rise4fun.com/Alive/K0YF

So the *old* (narrow) shift must be 'nuw' (it only shifts out zeros).


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1935
+    if (!matchAddressRecursively(NewShl, AM, Depth + 1))
+      return false;
+    break;
----------------
craig.topper wrote:
> lebedev.ri wrote:
> > What about the `insertDAGNode()` calls? We simply expect that they will be dropped afterwards?
> Should we restrict this to legal scale amounts? Can we just update the addressing mode and stop instead of calling matchAddressRecursively?
Yes, limiting to legal scale factors is probably better. Not sure about stopping the match here, but I'll try some experiments.

When I originally drafted this patch (a few days ago), this was allowing combining a narrow leading add with a wide trailing add by continuing the match. Something recent seems to have changed that though, so we invert the order of an add+shl.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60789/new/

https://reviews.llvm.org/D60789