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

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4184
@@ +4183,3 @@
+// the new and are removed by this function, while others (e.g. those whose path
+// from the load goes through a phi) are left for isel to potentially remove.
+//
----------------
Could you put some quotes around “and” or something?
I found it difficult to process as it is :).

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4216
@@ +4215,3 @@
+//
+// becomes:
+//
----------------
If I read the code correctly, this will happen only after a call to this optimization for each load.
Although the example is interesting, you should call out that it needs both loads to be optimized to end up in this situation, otherwise the expectations while reading the code are misled.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4240
@@ +4239,3 @@
+    User *LoadUser = *Load->user_begin();
+    if (!dyn_cast<Instruction>(LoadUser) ||
+        (cast<Instruction>(LoadUser)->getParent() == Load->getParent() &&
----------------
The dyn_cast is useless.
A user of an instruction must be an instruction or we are doing something weird.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4242
@@ +4241,3 @@
+        (cast<Instruction>(LoadUser)->getParent() == Load->getParent() &&
+         !dyn_cast<PHINode>(LoadUser)))
+      return false;
----------------
Just test getParent == getParent. Indeed, if you are in the same block, by construction the node cannot be a phi, since phis are the first instructions in the block.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4268
@@ +4267,3 @@
+    // For a PHI node, push all of its incoming values.
+    if (auto *Phi = dyn_cast<PHINode>(V)) {
+      for (auto *U : Phi->users())
----------------
You mean its users, right?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4274
@@ +4273,3 @@
+
+    if (auto *BinOp = dyn_cast<BinaryOperator>(V)) {
+      if (BinOp->getOpcode() == llvm::Instruction::And) {
----------------
As far as I can tell, you do not need to access anything specific to BinaryOperator.
I then suggest to turn this if…else… if… sequence into a switch:
switch (V->getOpcode()) {
case And:
…
default: return false;
}

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4284
@@ +4283,3 @@
+              isa<Instruction>(BinOp))
+            AndsToMaybeRemove.push_back(cast<Instruction>(BinOp));
+          continue;
----------------
BinOp must be an instruction at this point, given it has been constructed with dyn_cast<BinaryOperator>.
I.e., you don’t need to test that BinOp isa Instruction and you don’t need to cast BinOp to Instruction.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4308
@@ +4307,3 @@
+  // Avoid hoisting and 0x1 since it is unlikely to be folded by the target even
+  // if isLoadExtLegal says an i1 EXTLOAD is valid.
+  // Also avoid hoisting if we didn't see any ands with the exact DemandBits
----------------
Could you give more details here on the reason of the unlikeliness and what would happen if we do generate a i1 EXTLOAD?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4317
@@ +4316,3 @@
+  Type *ExtTy = Type::getIntNTy(Ctx, ActiveBits);
+  EVT ExtVT = TLI->getValueType(*DL, ExtTy);
+
----------------
TruncTy would be a better name, wouldn’t it?

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5053
@@ +5052,3 @@
+      if (TLI->enableExtLdPromotion() && !DisableExtLdPromotion)
+        Modified |= optimizeLoadExt(LI);
+
----------------
The enableExtLdPromotion was aimed at a different optimization.
I think it does not make much sense to reuse it here.

Wouldn’t it make sense to just do it?
That should always be a win, right?


http://reviews.llvm.org/D14584