[PATCH] D14584: [CodeGenPrepare] Create more extloads and fewer ands

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4242
@@ +4241,3 @@
+    if (cast<Instruction>(LoadUser)->getParent() == Load->getParent() &&
+        !dyn_cast<PHINode>(LoadUser))
+      return false;
----------------
gberry wrote:
> I don't think your comment about the user not being a phi in the same block is correct.  Consider a single block loop where a load inside the loop feeds a phi at the top of the loop.  I've added a test case for this (see test/Transforms/CodeGenPrepare/AArch64/free-zext.ll test_free_zext3).
Of course, you’re right! Please disregard that comment and thanks for adding a test case to cover that code.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4285
@@ +4284,3 @@
+      } else
+        return false;
+      break;
----------------
LLVM coding style is to go with early exits in those cases.
I.e.,
if (<inverted cond>)
  return false;
// Else, do the work.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4294
@@ +4293,3 @@
+      } else
+        return false;
+      break;
----------------
Ditto.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4308
@@ +4307,3 @@
+    }
+  }
+
----------------
gberry wrote:
> I tried to add more of an explanation here, let me know if it makes sense, or if you think this is a shortcoming in the AArch64 back-end that needs to be addressed.
That feels like a shortcoming in the AArch64 backend to me.
I could live with it for now if you file a PR so that we remember to look into it.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5061
@@ -4878,1 +5060,3 @@
+      Modified |= optimizeMemoryInst(I, I->getOperand(0), LI->getType(), AS);
+      return Modified;
     }
----------------
gberry wrote:
> I went ahead and removed this, though I'm not 100% happy with it.  I would like to avoid doing this work for targets that don't support any extloads at all (since it is wasted computation), but I couldn't find a good way of checking for that, so I was using enableExtLdPromotion as a proxy.
I see. In that case, we may consider adding a new target hook.

What is the impact of this optimization on the compile time anyway?

================
Comment at: test/CodeGen/AArch64/free-zext.ll:30
@@ +29,3 @@
+
+; Test for CodeGenPrepare::optimizeLoadExt()
+define i32 @test_free_zext3(i32* %ptr, i32* %ptr2, i32* %dst, i32 %c) {
----------------
Could you be more explicit on what case of optimizeLoadExt you are checking? E.g., test… when the phi is in the same block as the load.
This is usually useful when we have to update the tests.

This applies to all the added test cases.

================
Comment at: test/CodeGen/AArch64/free-zext.ll:55
@@ +54,3 @@
+; CHECK: ldrh [[REG:w[0-9]+]]
+; TODO: fix isel to remove final and XCHECK-NOT: and {{w[0-9]+}}, {{w[0-9]+}}, #0xffff
+; CHECK: ldrh [[REG:w[0-9]+]]
----------------
Make sure to file a PR when this land.


http://reviews.llvm.org/D14584