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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D56772#1365972 <https://reviews.llvm.org/D56772#1365972>, @anton-afanasyev wrote:

> Ping!


FYI: Generally accepted ping frequency is once per week.



================
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");
----------------
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)


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


================
Comment at: lib/CodeGen/MachineCSE.cpp:790
+      PREMap[MI] = MBB;
+    } else {
+      auto MBB1 = PREMap[MI];
----------------
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.


================
Comment at: lib/CodeGen/MachineCSE.cpp:812
+          if (!isProfitableToCSE(VReg, NewReg, MI, &NewMI)) {
+            NewMI.eraseFromParent();
+            continue;
----------------
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).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56772