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

[Tony Jiang via Phabricator via llvm-commits]

jtony marked 9 inline comments as done.
jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:55
+  ISELInstructionList ISELInstructions;
+  MachineFunction *MF;
+  bool IsTrueBlockRequired;
----------------
kbarton wrote:
> You've got two different references to MachineFunction: MF here and Fn on line 60
Good call, one of them is redundant, I removed all usage of Fn and use MF instead. 


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:69
+  MachineBasicBlock *NewSuccessor;
+  MachineBasicBlock *Successor;
+
----------------
kbarton wrote:
> 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. 
That's a good point, we should try to restrict the life of a variable as short as possible. Successor can be made a function scope variable while NewSuccessor can be passed around between functions.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:274
+      MI = BIL.erase(MI);
+      continue;
+    }
----------------
hfinkel wrote:
> Do we need to reset IsTrueBlockRequired and IsFalseBlockRequired here?
We don't need to reset them, the removed ISEL isn't considered for calculating the value of IsTrueBlockRequired  and IsFalseBlockRequired.


https://reviews.llvm.org/D23630