[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:68
+  void populateBlocks(BlockISELList &BIL);
+  void expandCanMergeISELs(BlockISELList &BIL);
+  void expandAndMergeISELs();
----------------
I meant to rename both `expandCanMergeISELs` with my previous comment. The name `expandCanMergeISELs` sounds to me like something called `expand` has the ability to merge ISELs - it's quite a confusing name.
The function takes a list of ISEL instructions, expands them into control flow AND it merges them (since they're already demonstrably mergeable).


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:74
+  ///  Is this instruction an ISEL or ISEL8?
+  bool isISEL(const MachineInstr &MI) const {
+    return (MI.getOpcode() == PPC::ISEL || MI.getOpcode() == PPC::ISEL8);
----------------
This is just a minor nit that you can choose to address or ignore, but this does not need any non-static members so I would make these queries static.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:255
+    // Special case 2, the two input registers used by ISEL are the same.
+    if (isSameRegister(TrueValue, FalseValue) && (BIL.size() == 1)) {
+      DEBUG(dbgs() << "Fold the ISEL instruction to an unconditonal copy.");
----------------
kbarton wrote:
> kbarton wrote:
> > I don't understand why this is only valid when there is one ISEL in the list.
> You also need to check for register 0. 
> This isn't valid if TrueValue or FalseValue is r0, since it has different semantics on the True/False paths. 
I think Kit raises valid points here and I think we should add comments to address them as any reader would reasonably have the same questions when looking at the code.

```
// Note 1: We favour merging ISEL expansions over folding a single one. If the
//         passed list has multiple merge-able ISEL's, we won't fold any.
// Note 2: There is no need to test for PPC::R0/PPC::X0 because the first input
//         for ISEL uses register class 'gprc_nor0' which explicitly excludes
//         PPC::R0 (and similarly for the 64-bit variants). So:
//         TrueVal.getReg() == FalseVal.getReg() => FalseVal.getReg() != PPC::R0
```


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:415
 void PPCPassConfig::addPreEmitPass() {
+  addPass(createPPCExpandISELPass(), false);
+  
----------------
kbarton wrote:
> hfinkel wrote:
> > Do you require the verifyAfter = false here? (I'm not sure why we have that for the others either). Can you test without it?
> Did this get done? 
> I agree it would be better to not use verifyAfter=false here unless it is needed for some reason. 
I don't know the characteristics of the verification, but I assume that it takes some non-zero compile time and that it will run verification in all build modes.
If this is the case, I am personally in favour of adding a call to `MachineFunction::verify()` at the end of `RunOnMachineFunction()` wrapped in an `#ifndef NDEBUG` so that we get the verification with asserts and in debug mode, but save the associated compile-time in release mode.


https://reviews.llvm.org/D23630