[PATCH] D34085: [PGO] Register promote profile counter updates

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

This is a really interesting patch. I haven't worked with SSAUpdater before, so I think it would be best if someone else reviewed that part of the patch.

On another note, do you know if counter promotion could be made to work with frontend-based instrumentation?



================
Comment at: include/llvm/Transforms/InstrProfiling.h:64
+  // vector of counter load/store pairs to be register promoted.
+  std::vector<std::pair<Instruction *, Instruction *>> PromotionCandidates;
+
----------------
It may be more convenient to introduce 'using LoadStorePair = ...'


================
Comment at: include/llvm/Transforms/InstrProfiling.h:75
+
+  /// Register promote counter loads and stores in loops.
+  void promoteCounterLoadStores(Function *F);
----------------
Could you write this with a dash, e.g 'Register-promote'


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:101
+cl::opt<bool> AtomicCounterUpdatePromoted(
+    "atomic-counter-update-promoted", cl::ZeroOrMore,
+    cl::desc("Do counter update using atomic fetch add "
----------------
Would cl::Optional be more appropriate?


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:111
+// setting this option can override the default behavior.
+cl::opt<bool> DoCounterPromotion("do-counter-promotion", cl::ZeroOrMore,
+                                 cl::desc("Do counter register promotion"),
----------------
The "do-counter-promotion" flag is ternary, and should be simplified. I'd prefer a cl::opt which specifies whether counter promotion is "allowed", and an InstrProfOptions field which specifies whether it is done. In promoteCounterLoadStores, you could write:
```
if (!AllowCounterPromotion || !Opts.DoCounterPromotion)
  return;
```



================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:148
 
+class PGOCounterPromoterHelper : public LoadAndStorePromoter {
+public:
----------------
It would help to have brief doxygen lines for the two new classes.


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:221
+    unsigned Promoted = 0;
+    for (auto Cand : Candidates) {
+
----------------
const auto &Cand?


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:313
+
+  for (auto LoadStore : PromotionCandidates) {
+    auto *CounterLoad = LoadStore.first;
----------------
const auto &LoadStore?


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:480
+  Inc->replaceAllUsesWith(Store);
+  PromotionCandidates.emplace_back(cast<Instruction>(Load), Store);
   Inc->eraseFromParent();
----------------
We shouldn't update the promotion candidates vector if counter promotion is disabled.


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:3
+; RUN: opt < %s --passes=pgo-instr-gen,instrprof -do-counter-promotion=true -S | FileCheck --check-prefix=PROMO %s
+ at g = common global i32 0, align 4
+
----------------
Is the global needed?


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:40
+  ret void
+; PROMO:  %pgocount.promoted = load {{.*}} @__profc_foo{{.*}} 0)
+; PROMO-NEXT: add 
----------------
You may need to add 'requires: asserts' to tests which refer to bb or variable names.


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
+
+}
----------------
In this test, the CFG looks like:
```
    2
   /  \
 4      5
 |     / \
 |    7    8
  \  /____/ 
    9 --- loop ---> 2
    |
   exit
```
Is this the simplest loop where ld/st promotion applies?


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
+
+}
----------------
vsk wrote:
> In this test, the CFG looks like:
> ```
>     2
>    /  \
>  4      5
>  |     / \
>  |    7    8
>   \  /____/ 
>     9 --- loop ---> 2
>     |
>    exit
> ```
> Is this the simplest loop where ld/st promotion applies?
Could you add checks for the operands to the adds? It would be useful to have checks for the PHIs incremented in the loop body, to ensure they make their way to the correct 'add'.


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
+
+}
----------------
vsk wrote:
> vsk wrote:
> > In this test, the CFG looks like:
> > ```
> >     2
> >    /  \
> >  4      5
> >  |     / \
> >  |    7    8
> >   \  /____/ 
> >     9 --- loop ---> 2
> >     |
> >    exit
> > ```
> > Is this the simplest loop where ld/st promotion applies?
> Could you add checks for the operands to the adds? It would be useful to have checks for the PHIs incremented in the loop body, to ensure they make their way to the correct 'add'.
Could you add a check-not for further uses of __profc*?


================
Comment at: test/Transforms/PGOProfile/counter_promo_mexits.ll:72
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}4)
+
+
----------------
It would help to have 'check-nots' for further updates to __profc* in this test as well.


================
Comment at: test/profile/Linux/counter_promo_for.c:15
+// RUN: llvm-profdata show --counts --all-functions %t.nopromo.profdata  > %t.nopromo.dump
+// RUN: diff %t.promo.profdata %t.nopromo.profdata
+
----------------
Why are the IR checks needed in this test? Imho the interesting thing about this test is the "diff", and not the IR checks, which are probably better off in test/Transforms/PGOProfile.


================
Comment at: test/profile/Linux/counter_promo_while.c:39
+// NOPROMO-NEXT: add
+// NOPROMO-NEXT: store{{.*}}@__profc_foo{{.*}} 2){{.*}}
+  int i = 0;
----------------
I have the same comment about the IR checks here.


================
Comment at: test/profile/Linux/counter_promo_while.c:41
+  int i = 0;
+  while (i < N) {
+    if (i < n + 1)
----------------
Would placing a nested for loop here increase test coverage?


https://reviews.llvm.org/D34085