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

[Kit Barton via Phabricator via llvm-commits]

kbarton accepted this revision.
kbarton added a comment.
This revision is now accepted and ready to land.

Aside from updating two comments, this LGTM.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:260
+    // input for ISEL uses register class 'gprc_nor0' which explicitly excludes
+    // PPC::R0 (and similarly for the 64-bit variants). So:
+    // useSameRegister(TrueValue, FalseValue)  => FalseValue.getReg() != PPC::R0
----------------
nemanjai wrote:
> kbarton wrote:
> > There is currently a bug filed against the PPC backend for this (https://llvm.org/bugs/show_bug.cgi?id=31065). Once this bug is fixed, the register class will need to include r0.
> > Please include a check for R0 here as well, to ensure correctness. 
> The register class can't include PPC::R0/PPC::X0. It must include PPC::ZERO/PPC::ZERO8 instead. The bug mentioned isn't valid (I've added a clarifying comment to it). We certainly do allocate PPC::ZERO/PPC::ZERO8 for the ISEL instruction when the true operand comes from `LI 0` (i.e. it's an immediate zero). This is done in `PPCInstrInfo::FoldImmediate()`.
OK, I understand.
In that case, please change the comment to:

Note 2: There is no need to test for PPC::R0/PPC::X0 because PPC::ZERO/PPC::ZERO8 will be used for the first operand if the value is meant to be zero. In this case, the useSameRegister method will return false, thereby preventing this ISEL from being folded. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:347
+    for (MachineInstr &MI : *MBB)
+      LPR.stepForward(MI, Clobbers);
+    for (auto &LI : LPR)
----------------
kbarton wrote:
> According to the documentation for stepForward, it is not the preferred method for calculating liveness. Instead, stepBackward should be used instead. 
> Did you try using stepBackward? If not, please change this to use stepBackward.
Based on an off-line conversanion, stepBackwark doesn't work.
Please add a comment here saying why stepForward is being used. 


https://reviews.llvm.org/D23630