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

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:33
+    GenerateISEL("ppc-gen-isel",
+                 cl::desc("Enable generating the ISEL instruction"),
+                 cl::init(true), cl::Hidden);
----------------
Should probably be changed to something along the lines of
`Disable generating the ISEL instruction on 64-bit non-Darwin PPC targets.`


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:184
+
+bool PPCExpandISEL::expandISEL(MachineInstr &MI) {
+  assert(isISEL(MI) && "Expecting an ISEL instruction");
----------------
I'm not sure it makes sense for this function to have a `bool` return value since it can never return `false`. This should likely be a `void` function.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:302
+    NewSuccessor->addLiveIn(TrueValue.getReg());
+    NewSuccessor->addLiveIn(FalseValue.getReg());
+  }
----------------
One of these may be redundant. I think if the code is reorganized to keep things together, it will be clearer what needs to be done. What I mean is that if we group together things we do if we need a true block and group together things we do if we need a false block, the logic will be easier to follow.


https://reviews.llvm.org/D23630