[PATCH] D26524: [AArch64] Sink sext when foldable in user GEPs

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/AArch64/AArch64AddressTypePromotion.cpp:140
+  /// Sink the sign extension to user blocks.
+  void performSink(Instruction *SExtInst, Instructions &GEPsInOtherBlock);
 };
----------------
"performSink" is so generic - can we call it something more specific like "sinkSExt" ?


================
Comment at: lib/Target/AArch64/AArch64AddressTypePromotion.cpp:376
 
+bool AArch64AddressTypePromotion::isAllFoldable(
+    Instruction *SExtInst, Instructions &GEPsInOtherBlock) {
----------------
Same here - "isAllFoldable" - can we make that clear we're referring to SExts?


================
Comment at: lib/Target/AArch64/AArch64AddressTypePromotion.cpp:385
+    if (!GEPInst) {
+      GEPsInOtherBlock.clear();
+      return false;
----------------
You shouldn't need these clear() operations, because this array will not be used if we return false.


================
Comment at: lib/Target/AArch64/AArch64AddressTypePromotion.cpp:394
+    gep_type_iterator GTI = gep_type_begin(GEPInst);
+    for (unsigned i = 1, e = GEPInst->getNumOperands(); i != e; ++i, ++GTI) {
+      if (GEPInst->getOperand(i) == SExtInst) {
----------------
Shouldn't this function only succeed if the SExt was the *last* operand in the GEP? We can't represent a GEP such as:

  %0 = sext i16 %idx to i32
  %1 = gep inbounds i32* %base, i32 %0, i32 6

So I think this function is not conservative enough.


https://reviews.llvm.org/D26524