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

[Kit Barton via Phabricator via llvm-commits]

kbarton added a comment.

There are still a few things that need to get cleaned up.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:84
+  /// Are the two operands using the same register?
+  bool isSameRegister(const MachineOperand &Op1, const MachineOperand &Op2) {
+    return (Op1.getReg() == Op2.getReg());
----------------
Technically, I think this should be called useSameRegister


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:113
+  /// The -ppc-gen-isel option is set to true by default. Which means the ISEL
+  /// insturction is still generated by default on targets that support them.
+  ///
----------------
insturction -> instruction


================
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.");
----------------
I don't understand why this is only valid when there is one ISEL in the list.


================
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:
> 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. 


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:415
 void PPCPassConfig::addPreEmitPass() {
+  addPass(createPPCExpandISELPass(), false);
+  
----------------
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. 


https://reviews.llvm.org/D23630