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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:11
+// A pass that expands the ISEL instruction into an if-then-else sequence
+//
+//===----------------------------------------------------------------------===//
----------------
hfinkel wrote:
> Please note here that this pass is intended to be run post-RA only.
This wasn't addressed. I think it's important to note that since we need to have physical registers here.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:36
+
+// If ppc-gen-isel=false is set, it will disable generating the ISEL
+// instruction on 64-bit non-Darwin PPC targets, for other targets,
----------------
This comment is no longer accurate.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:108
+
+  const TargetInstrInfo *TII;
+
----------------
Why does this need to be public?


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:120
+  /// ISEL insturctions are still generated by default on all platforms.
+  /// instruction, regardless of the architecture.
+  ///
----------------
Fragment. Please revise this comment to complete sentences and to account for the new semantics (i.e. interaction with -mno-isel/-mattr=-isel).


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:132
+  bool runOnMachineFunction(MachineFunction &MF) override {
+    DEBUG(dbgs() << "Function: "; MF.dump(); dbgs() << "\n");
+    if (!isExpandISELEnabled(MF))
----------------
I would move this down. We don't want to dump out an entire MachineFunction if we're not planning to do anything with it.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:159
+
+bool PPCExpandISEL::isExpandISELEnabled(const MachineFunction &MF) {
+  if (!GenerateISEL)
----------------
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.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:247
+    assert(isISEL(**MI) && "Expecting an ISEL instruction");
+    DEBUG(dbgs() << "**MI: " << **MI << "\n");
+
----------------
I don't think the name of the variable is useful to the consumer of the debug output. Probably something like:
`DEBUG(dbgs() << "ISEL: " << **MI << "\n");`


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:273
+      (*MI)->eraseFromParent();
+      MI = BIL.erase(MI);
+      continue;
----------------
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.


================
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.");
----------------
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?


================
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++)
----------------
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.


https://reviews.llvm.org/D23630