[PATCH] D30431: [PowerPC] MachineSSA pass to reduce the number of CR-logical operations

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCMachineBasicBlockUtils.h:93
+/// MachineBranchProbabilityInfo isn't null.
+static bool splitMBB(MachineInstr *OrigBranch, MachineInstr *SplitBefore,
+                     MachineInstr *SplitCond, bool InvertNewBranch,
----------------
jtony wrote:
> It looks to me this static function uses too many parameters, which makes it a little bit hard to understand. Is it possible for us to reduce the number of parameters used here? One way I could think of is to make this a member function of class PPCReduceCRLogicals. In that case we could make some of the parameters member of the class, so that we don't have to pass them around. But it is totally up to you to decide whether the effort is worthwhile or not.
I don't want to make it a member variable as I think this is useful for other purposes than just this pass. However, I think it might be useful to wrap all the parameters into a struct that we'll build up and pass.


================
Comment at: lib/Target/PowerPC/PPCReduceCRLogicals.cpp:220
+
+    // FIXME: remove the variable, just return the result of simplifyCode()
+    bool Changed = false;
----------------
jtony wrote:
> Can we do what the `FIXME` suggested here in this patch?  Since the community doesn't like us putting `FIXME` in the code.
I meant to remove this FIXME. Thanks for pointing it out.


Repository:
  rL LLVM

https://reviews.llvm.org/D30431