[PATCH] D64551: [X86] EltsFromConsecutiveLoads - support common source loads

Thu Jul 18 05:43:54 PDT 2019

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

It's worth noting here in the review that this patch depends on the dereferenceable attribute (see D64205 <https://reviews.llvm.org/D64205>), and that attribute could change meaning as part of the larger changes related to the Attributor pass (D63243 <https://reviews.llvm.org/D63243>).
Based on current definitions, I think this is correct and allowable, so LGTM.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7505
+// Recurse to find a LoadSDNode source and the accumulated ByteOffest.
+static bool FindEltLoadSrc(SDValue Elt, LoadSDNode *&Ld, int64_t &ByteOffset) {
+  if (ISD::isNON_EXTLoad(Elt.getNode())) {
----------------
formatting:
FindElt... --> findElt

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7519-7521
+        Elt.getConstantOperandAPInt(1).urem(8) == 0 &&
+        FindEltLoadSrc(Elt.getOperand(0), Ld, ByteOffset)) {
+      ByteOffset += Elt.getConstantOperandVal(1) / 8;
----------------
Inconsistent methods for getting the constant (APInt vs. uin64_t). I don't think a logic difference is possible given other limits of LLVM, so go with getConstantOperandVal() in both places?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64551