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

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:34
@@ +33,3 @@
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::Hidden);
+
----------------
I think it's clearer if you explicitly initialize this to false (presumably that's the default).

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:129
@@ +128,3 @@
+
+  if (MF.getSubtarget<PPCSubtarget>().isSVR4ABI() &&
+      MF.getSubtarget<PPCSubtarget>().isPPC64())
----------------
The doxygen comment for this function states that we want to expand the isel instructions on Power7 and Power8 processors but this actually relies on different conditions. I am not entirely clear on whether the SVR4 ABI only applies on those processors, but the conditions seem to be mismatched.

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:143
@@ +142,3 @@
+    if (!thisBlockISELs.empty())
+      ISELInstructions.insert(std::make_pair(MBB.getNumber(), thisBlockISELs));
+  }
----------------
Is there any concern that there may be some [pathological] cases where this would end up copying an unreasonably large vector (i.e. if the BB contains many isel instructions and has heap-allocated storage)?

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:148
@@ +147,3 @@
+
+void PPCExpandISEL::DumpISELInstructions() const {
+  for (ISELInstructionList::const_iterator I = ISELInstructions.begin(),
----------------
Should this whole function be in an
`#ifndef _NDEBUG`
block? And of course the call.

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:164
@@ +163,3 @@
+       BlockList != ListEnd; ++BlockList) {
+    DEBUG(dbgs() << "Expanding ISEL instructions in block " << BlockList->first
+                 << "\n");
----------------
Minor nit: The block numbers are dumped as "BB#<num>" above, but here they're dumped as "block <num>".

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:182
@@ +181,3 @@
+
+  assert((Opcode == PPC::ISEL || Opcode == PPC::ISEL8) &&
+         "Expecting an ISEL instruction");
----------------
Not that this is likely to ever change, but perhaps for the sake of consistency this assert should be:
```
assert(isISEL(MI) && "Expecting an ISEL instruction");
```

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:187
@@ +186,3 @@
+
+  for (unsigned i = 0; i < NumOperands; i++) {
+    DEBUG(dbgs() << "Operand " << i << ": " << MI.getOperand(i) << "\n");
----------------
Again, when the only statement[s] in the loop will be compiled out without asserts, I think the whole loop should be in a DEBUG() block. Mind you, I don't fully follow why we're dumping the operands separately after we've already dumped the MachineInstr - especially since they'll be dumped again below (Dest, TrueValue, False, ConditionRegister).

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:212
@@ +211,3 @@
+  MachineBasicBlock *NewSuccessor =
+      &MI != (MachineInstr *)MBB->getLastNonDebugInstr()
+          ? Fn->CreateMachineBasicBlock(LLVM_BB)
----------------
Does this cast work (i.e. converting an iterator to a pointer)? 

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:221
@@ +220,3 @@
+  if (!NewSuccessor) {
+    assert(MBB->canFallThrough() && "This block should fall through!\n");
+    for (MachineBasicBlock::succ_iterator succ_it = MBB->succ_begin();
----------------
I don't know the details of this so this may just be a ridiculous suggestion, but will this not assert if the isel is the very last instruction before returning from the function? Perhaps that would be impossible because the branch-to-link-register would be last. Except perhaps if this is a "noreturn" function (in which case, I'm not sure why there'd be an isel at the end).

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:222
@@ +221,3 @@
+    assert(MBB->canFallThrough() && "This block should fall through!\n");
+    for (MachineBasicBlock::succ_iterator succ_it = MBB->succ_begin();
+         succ_it != MBB->succ_end(); succ_it++) {
----------------
Minor nit: I think that "succ_it" (although a cool variable name :)) breaks with variable naming conventions (camel vs. underscores).

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:266
@@ +265,3 @@
+  // Copy the result into the destination if the condition is true
+  // MachineInstr *i1=
+  BuildMI(*TrueBlock, TrueBlockI, dl, TII->get(PPC::ADDI8))
----------------
Probably forgot to delete this line. Same applies to similar lines below.

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:267
@@ +266,3 @@
+  // MachineInstr *i1=
+  BuildMI(*TrueBlock, TrueBlockI, dl, TII->get(PPC::ADDI8))
+      .addOperand(Dest)
----------------
I think for both cases where you add this instruction, a check should be performed whether the two registers are the same. If they are, you don't need the instruction. Also, just out of curiosity, is there any significance to the choice to use ADDI vs. OR (which I think is what we normally use for reg-to-reg copies)?

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:271
@@ +270,3 @@
+      .addOperand(MachineOperand::CreateImm(0))
+      .getInstr();
+
----------------
This is probably not needed since you're not saving a pointer to the instruction. Same applies below.

================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:284
@@ +283,3 @@
+
+  // Conditional branch to the TrueBlock
+  // MachineInstr *i2 =
----------------
I wonder if there's anything available at this stage in the compilation that can be used for some heuristics to choose which block we should branch to and which should be a fall-through block?


https://reviews.llvm.org/D23630