[PATCH] D23630: [PPC] Expand ISEL instruction into if-then-else sequence

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

Thanks for all of the updates! Some more things...

In general, our policy is that comments are complete sentences, starting with a capital letter and ending with a period (or other punctuation). Please audit all comments and adjust accordingly.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:255
+
+    // if at least one of the ISEL instructions satisfy the following
+    // condition, we need the True Block:
----------------
if -> If


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:264
+    bool IsADDIInstRequired =
+        ((Dest.getReg() != TrueValue.getReg()) || (TrueValue.getReg() == 0));
+    bool IsORIInstRequired = (Dest.getReg() != FalseValue.getReg());
----------------
You need to check against PPC::X0 and PPC::R0, not the literal 0. Please create some .mir test cases for these.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:274
+      MI = BIL.erase(MI);
+      continue;
+    }
----------------
Do we need to reset IsTrueBlockRequired and IsFalseBlockRequired here?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:280
+    if ((TrueValue.getReg() == FalseValue.getReg()) &&
+        (TrueValue.getReg() != 0) && (BIL.size() == 1)) {
+      DEBUG(dbgs() << "Fold the ISEL insturction to an unconditonal copy.");
----------------
Same here (don't compare to the literal 0).


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:309
+  if (!NewSuccessor) {
+    assert(MBB->canFallThrough() && "This block should fall through!\n");
+    for (MachineBasicBlock::succ_iterator SuccIt = MBB->succ_begin();
----------------
I think that, in rare circumstances, this assert would fire. It would happen if you had a block that was the last block in a function which ended with an "unreachable" and the instruction before that were an isel. I think you need to handle this case. I think that you should be able to create a .mir test case for this.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:398
+    bool IsADDIInstRequired =
+        ((Dest.getReg() != TrueValue.getReg()) || (TrueValue.getReg() == 0));
+    bool IsORIInstRequired = (Dest.getReg() != FalseValue.getReg());
----------------
Don't compare with literial zero here either.


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:415
 void PPCPassConfig::addPreEmitPass() {
+  addPass(createPPCExpandISELPass(), false);
+  
----------------
Do you require the verifyAfter = false here? (I'm not sure why we have that for the others either). Can you test without it?


https://reviews.llvm.org/D23630