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

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 29 14:51:06 PST 2016


nemanjai added a comment.

None of the comments indicate a serious issue that requires a major revision. However, there are a number of comments to be addressed and I think it'd be better if you published another revision that addresses them.

However, if other reviewers feel this is ready to commit, I certainly won't object.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:61
+
+  // List of ISEL instructions, sorted by MBB.
+  ISELInstructionList ISELInstructions;
----------------
Minor nit: I'd just state in the comment what this actually is - "A map of MBB numbers to their lists of contained ISEL instructions."


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:70
+  void populateBlocks(BlockISELList &BIL);
+  void expandCanMergeISELs(BlockISELList &BIL);
+  void expandISELInstructions();
----------------
I don't think the name clearly reflects what the function does. Perhaps `expandAndMergeISELs`.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:85
+
+  ///  Is the two operands using the same register?
+  bool isSameRegister(MachineOperand &Op1, MachineOperand &Op2) {
----------------
"Are the two operands..."


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:86
+  ///  Is the two operands using the same register?
+  bool isSameRegister(MachineOperand &Op1, MachineOperand &Op2) {
+    return (Op1.getReg() == Op2.getReg());
----------------
Please make these references const, we don't want to modify them.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:165
+    if (!thisBlockISELs.empty())
+      ISELInstructions.insert(std::make_pair(MBB.getNumber(), thisBlockISELs));
+  }
----------------
I wonder if we should maybe have an assert here that the insert actually inserts something (i.e. we don't somehow end up with duplicate MBB numbers). Of course, this is not likely to happen, but if it did, we'd have unpredictable behaviour.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:181
+bool PPCExpandISEL::canMerge(MachineInstr *PrevPushedMI, MachineInstr *MI) {
+  bool IsSameCR =
+      isSameRegister(PrevPushedMI->getOperand(3), MI->getOperand(3));
----------------
I would simplify this function with early exits rather than initializing boolean variables you'll only query once:
```
if (!(isSameRegister(...))
  return false;
// more init - come to think of it, you may want to just get rid of them and cast to iterator in the if below

if (std::prev(MBBI) != PrevPushedMBBI) {
  NumNonContiguous++;
  return false;
}

NumContiguous++;
return true;
```


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:226
+  auto MI = BIL.begin();
+  while (MI != BIL.end()) {
+    assert(isISEL(**MI) && "Expecting an ISEL instruction");
----------------
No need to evaluate `BIL.end()` on every iteration. Or is it needed because of the potential call to erase()?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:293
+  MachineBasicBlock::iterator MBBI = (*BIL.back()); // implicit conversion
+  NewSuccessor = (MBBI != MBB->getLastNonDebugInstr() || !MBB->canFallThrough())
+                     ? MF->CreateMachineBasicBlock(LLVM_BB)
----------------
Perhaps a quick comment above this line. Sure you know why we're creating the basic block now, but you (or anyone else) will have to think about this condition a fair bit to understand why.
Maybe something like:
```
// Another basic block is needed to move the instructions that follow this ISEL.
// If the ISEL is the last instruction in a block that can't fall through,
// we need another block to branch to.
```


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:304
+  if (!NewSuccessor) {
+    for (MachineBasicBlock::succ_iterator SuccIt = MBB->succ_begin();
+         SuccIt != MBB->succ_end(); SuccIt++) {
----------------
Why is this not a range-based for loop? The MBB should have a way to get an iterator_range for its successors. Perhaps something like `MBB->successors()`


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:337
+    // Copy the liveIn to new successor.
+    for (MachineBasicBlock::livein_iterator LIT = MBB->livein_begin();
+         LIT != MBB->livein_end(); LIT++)
----------------
Again, there must be a way to make this a range-based for loop. Perhaps `MBB->liveins()`?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:348
+  // as successors!
+  MBB->addSuccessor(IsTrueBlockRequired ? TrueBlock : Successor);
+  MBB->addSuccessor(IsFalseBlockRequired ? FalseBlock : Successor);
----------------
It seems that you only need to add `Successor` as a successor to `MBB` if either the `TrueBlock` or the `FalseBlock` are not required. If so, these don't need to be executed unconditionally here - they can be sunk into the respective if statement.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:386
+    // are both R0, we need the True Block.
+    bool IsADDIInstRequired = !isSameRegister(Dest, TrueValue);
+    bool IsORIInstRequired = !isSameRegister(Dest, FalseValue);
----------------
Perhaps I've lost track here since the patch is fairly large, but don't you already have this available in `IsTrueBlockRequired`? Similarly with `IsFalseBlockRequired` below.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:404
+      // Add the LiveIn registers required by false block.
+      FalseBlock->addLiveIn(Dest.getReg());
+      FalseBlock->addLiveIn(FalseValue.getReg());
----------------
Why? I don't see the need for a register you're about to define to be added as live-in.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:411
+      NewSuccessor->addLiveIn(Dest.getReg());
+      NewSuccessor->addLiveIn(TrueValue.getReg());
+      NewSuccessor->addLiveIn(FalseValue.getReg());
----------------
This seems a bit suspicious. If the ISEL you're actually expanding was a KILL for the True/False Register[s], it seems wrong to add it as live-in to the successor.

I suppose it doesn't necessarily hurt since you're not actually setting kill flags on the instructions you're adding. The only way I could see this causing a problem is if something that runs after this is looking for free registers. Of course, this isn't impossible. I think it's quite possible that prologue/epilogue insertion happens after this and it may need to find an unused register. Since you've [possibly] eliminated the kill flags, the register scavenger will not find these and we may emit sub-optimal code if we're shrink-wrapping.


================
Comment at: lib/Target/PowerPC/PPCSubtarget.h:301
 
-  bool enableEarlyIfConversion() const override { return hasISEL(); }
+  bool enableEarlyIfConversion() const override { return true; }
 
----------------
I didn't check, but presumably the base class returns false for this? If not, just get rid of the override. If so, put a comment as to why we always want to run EarlyIfConversion.


================
Comment at: test/CodeGen/PowerPC/expand-contiguous-isel.ll:123
+; CHECK-LABEL: @_Z3fn1N4llvm9StringRefE
+; CHECK-GEN-ISEL-TRUE: isel r3, r3, r3, 2
+; CHECK-GEN-ISEL-TRUE: isel r6, r7, r6, 2
----------------
I don't think this particular register allocation is guaranteed. Perhaps change all of these to something like:

; CHECK-GEN-ISEL-TRUE: isel [[SAME:r[0-9]+]], [[SAME]], [[SAME]]


================
Comment at: test/CodeGen/PowerPC/expand-isel-2.mir:35
+    %r3 = ISEL %zero, %r4, %cr0gt
+    ; CHECK: BC %cr0gt, %bb
+    ; CHECK: %bb
----------------
All the %bb without a number is confusing. Can you change these to something like
`; CHECK: BC %cr0gt, [[TRUE:%bb[0-9]+]]`
and so on?


https://reviews.llvm.org/D23630





More information about the llvm-commits mailing list