[PATCH] D23630: [PPC] Expand ISEL instruction into if-then-else sequence
Tony Jiang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 15 06:58:13 PST 2016
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;
> 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;
> 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);
> 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.
More information about the llvm-commits