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

[Kit Barton via Phabricator via llvm-commits]

kbarton added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:55
+  ISELInstructionList ISELInstructions;
+  MachineFunction *MF;
+  bool IsTrueBlockRequired;
----------------
You've got two different references to MachineFunction: MF here and Fn on line 60


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:62
+  MachineFunction::iterator It;
+  const BasicBlock *LLVM_BB;
+  MachineBasicBlock *TrueBlock;
----------------
Why do we need an LLVM_BB? I would have expected we're only working with MachineBasicBlocks at this point.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:69
+  MachineBasicBlock *NewSuccessor;
+  MachineBasicBlock *Successor;
+
----------------
In general I don't like this approach of caching all of these intermediate values here. Given the algorithm iterates over lists of ISELs and does expansions somewhat different each time, you need to be very careful that all of these are set/reset between each iteration to ensure that the values are not stale. My previous experiences have taught me that this can be both confusing and error prone. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:188
+
+bool PPCExpandISEL::canMerge(MachineInstr *&PrevPushedMI, MachineInstr *&MI) {
+  bool IsSameCR =
----------------
I don't see why these parameters need to references to MachineInstr *, why can't they just be references to MachineInstr? 
I think the preference is to only use pointers if the value can be null for some reason. If it cannot be null, then it's better to use references.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:281
+        (TrueValue.getReg() != 0) && (BIL.size() == 1)) {
+      DEBUG(dbgs() << "Fold the ISEL insturction to an unconditonal copy.");
+      NumFolded++;
----------------
insturction -> instruction


https://reviews.llvm.org/D23630