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

[Tony Jiang via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:34
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::init(true), cl::Hidden);
+
----------------
jtony wrote:
> Shall we make this option visible to all users?
Discussed in the scrum, we want it to be hidden because the user can use the FE option -misel and -mno-isel to control the generation of ISEL instructions.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:108
+
+  const TargetInstrInfo *TII;
+
----------------
nemanjai wrote:
> Why does this need to be public?
Good call, it doesn't need to be public, have made it class private.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:159
+
+bool PPCExpandISEL::isExpandISELEnabled(const MachineFunction &MF) {
+  if (!GenerateISEL)
----------------
nemanjai wrote:
> I don't really understand the purpose for this function any longer. Why do we want two different options to disable generating ISEL's?
> With this we have -mno-isel/-mattr=-isel as well as -ppc-generate-isel=false.
Good question, we need the -ppc-generate-isel only for our convenient, without it we couldn't control the IR and MIR test cases to expand ISEL in the  BE,  the FE options misel and mno-isel is for the users, the ppc-gen-isel option is set to hidden are only used by us. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:273
+      (*MI)->eraseFromParent();
+      MI = BIL.erase(MI);
+      continue;
----------------
nemanjai wrote:
> I suppose you've considered whether the erase here invalidates the iterator...
> Perhaps setting MI to the result of the erase keeps the iterator valid. If so, maybe a quick comment to that end.
Exactly, a lot of people made errors about this, I fixed one such bug before in the FE. Good call, I will add a comment to that end.


================
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.");
----------------
nemanjai wrote:
> hfinkel wrote:
> > Same here (don't compare to the literal 0).
> Just out of curiosity, is the comparison even needed? Wouldn't an ISEL that looks like
> `isel 5, 0, 0, 1`
> actually be a MachineInstr like
> `ISEL %X5, %ZERO8, %X0, %CR0LT`
> or something along those lines?
The  (TrueValue.getReg() != PPC::R0) parted is redundant, removed.


================
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();
----------------
hfinkel wrote:
> 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.
Good call! Fixed and tested with a MIR file:  expand-isel-6.mir


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:344
+    // Copy the liveIn to new successor
+    for (MachineBasicBlock::livein_iterator LIT = MBB->livein_begin();
+         LIT != MBB->livein_end(); LIT++)
----------------
nemanjai wrote:
> I am wondering if this is safe to do.
> If there's a KILL for one of the live-ins in this MBB prior to the ISEL being expanded (or the ISEL itself is a KILL), would we violate some kind of invariant? Basically adding a live-in to a block when it isn't live out of the now truncated MBB.
You are right we don't need to copy all the LiveIn registers, but it's hard to tell which ones we should copy and which shouldn't, we simply called everything to the NewSuccessor. I think it is just some performance issue, won't cause any problem as far as I know. I also tested adding an redudant register to an MIR file, it doesn't cause issue. 


https://reviews.llvm.org/D23630