[PATCH] D56772: [MIR] Add simple PRE pass to MachineCSE

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 8 inline comments as done.
anton-afanasyev added a comment.

In D56772#1367096 <https://reviews.llvm.org/D56772#1367096>, @hfinkel wrote:

> In D56772#1365972 <https://reviews.llvm.org/D56772#1365972>, @anton-afanasyev wrote:
>
> > Ping!
>
>
> FYI: Generally accepted ping frequency is once per week.


Ok, thanks!



================
Comment at: lib/CodeGen/MachineCSE.cpp:54
 STATISTIC(NumCSEs,      "Number of common subexpression eliminated");
+STATISTIC(NumPREs,      "Number of partial redundant expression \
+transformed to fully redundant");
----------------
hfinkel wrote:
> Our general style is not to use line continuations like this, but instead rely on adjacent string concatenation, so this would be:
> 
>   STATISTIC(NumPREs,      "Number of partial redundant expression"
>                               " transformed to fully redundant");
> 
> (where the starting quotes are vertically aligned)
Ok, fixed.


================
Comment at: lib/CodeGen/MachineCSE.cpp:754
 
+// Stronger conditions are used for PREed instrs rather than for CSE ones
+bool MachineCSE::isPRECandidate(MachineInstr *MI) {
----------------
hfinkel wrote:
> Comments should be full sentences and end with a period (or other punctuation). Also, please explain *why* the conditions are stronger in this case.
Ok, changed.


================
Comment at: lib/CodeGen/MachineCSE.cpp:790
+      PREMap[MI] = MBB;
+    } else {
+      auto MBB1 = PREMap[MI];
----------------
hfinkel wrote:
> It looks like this could be:
> 
>   if (!PREMap.count(MI)) {
>     PREMap[MI] = MBB;
>     continue;
>   }
> 
> and then you don't need the else and extra associated indentation.
Thanks, that is better! Fixed.


================
Comment at: lib/CodeGen/MachineCSE.cpp:812
+          if (!isProfitableToCSE(VReg, NewReg, MI, &NewMI)) {
+            NewMI.eraseFromParent();
+            continue;
----------------
hfinkel wrote:
> It would be better to not create, and then delete, instructions speculatively. isProfitableToCSE does look at the parent blocks of the instructions, but we could abstract that by:
> 
>  1. Making isProfitableToCSE also take the blocks of the instructions as arguments; explain that isProfitableToCSE treats the instructions as though they are in the specified parent blocks (even if they're not currently).
>  2. Make an overload isProfitableToCSE which retains the current behavior (where the parent blocks come from the instructions themselves) which calls the underlying implementation from (1).
Thanks, changed. Actually isProfitableToCSE() doesn't need CSMI (new instr) as argument, only its basic block, so just changed function arglist.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56772/new/

https://reviews.llvm.org/D56772