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

Tony Jiang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jan 1 17:29:12 PST 2017


jtony marked 13 inline comments as done.
jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:165
+    if (!thisBlockISELs.empty())
+      ISELInstructions.insert(std::make_pair(MBB.getNumber(), thisBlockISELs));
+  }
----------------
nemanjai wrote:
> 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.
Sorry , I am not clear what kind of assertion we should insert here, can you be more specific?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:226
+  auto MI = BIL.begin();
+  while (MI != BIL.end()) {
+    assert(isISEL(**MI) && "Expecting an ISEL instruction");
----------------
nemanjai wrote:
> No need to evaluate `BIL.end()` on every iteration. Or is it needed because of the potential call to erase()?
I think the BIL.end() is needed here, one reason is we may call erase, also even we don't call erase(), we need it to tell us when to finish the loop.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:348
+  // as successors!
+  MBB->addSuccessor(IsTrueBlockRequired ? TrueBlock : Successor);
+  MBB->addSuccessor(IsFalseBlockRequired ? FalseBlock : Successor);
----------------
nemanjai wrote:
> 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.
I can merge these two ternary expression with the following two if guards by adding two more else, but I prefer to leave it as it is unless for some other solid reason.


================
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);
----------------
nemanjai wrote:
> 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.
I agree this patch is really large and kind of hard to follow the whole logic.  IsTrueBlockRequired is used to guard whether generate the True Block, while IsADDIInstRequired only guards whether we should generate one ADDI instruction. IsTrueBlockRequired  = IsADDIInstRequired(1) OR IsADDIInstRequired(2)  ... OR IsADDIInstRequired(N). Similarly for IsFalseBlockRequired  and IsORIInstRequired.


https://reviews.llvm.org/D23630





More information about the llvm-commits mailing list