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

[David Li via Phabricator via llvm-commits]

davidxl marked 7 inline comments as done.
davidxl added a comment.

Regarding turning this on for FE instrumentation, it can be done, but probably as a follow up (which also needs loop canonlicalization and rotation). More coverage test performance data (possibly at O0) also need to be collected.



================
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 "
----------------
vsk wrote:
> Would cl::Optional be more appropriate?
Sometimes the build system can append multiple instances of a common flag to the command line. Without ZeroOrMore, driver reports error.  


================
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"),
----------------
vsk wrote:
> 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;
> ```
> 
We need the ability to use this option to override (either force it to be on or off) the default pipeline option. Your proposal does not seem to accomplish that.


================
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
+
----------------
vsk wrote:
> Is the global needed?
removed.


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:40
+  ret void
+; PROMO:  %pgocount.promoted = load {{.*}} @__profc_foo{{.*}} 0)
+; PROMO-NEXT: add 
----------------
vsk wrote:
> You may need to add 'requires: asserts' to tests which refer to bb or variable names.
The name of the counter vars seem to be kept with the non-asserts compiler. Any idea why?


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
+
+}
----------------
vsk wrote:
> 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*?
The purpose is to test multiple counters hoisted 


================
Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
+
+}
----------------
davidxl wrote:
> vsk wrote:
> > 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*?
> The purpose is to test multiple counters hoisted 
Done. 


================
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
+
----------------
vsk wrote:
> 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.
It is end-to-end test to make sure every single step in the piplineline (from FE ) is covered. If the transformation does not happen, it makes no sense to do DIFF any more.  The LLVM tests only cover the lowering part.


================
Comment at: test/profile/Linux/counter_promo_while.c:39
+// NOPROMO-NEXT: add
+// NOPROMO-NEXT: store{{.*}}@__profc_foo{{.*}} 2){{.*}}
+  int i = 0;
----------------
vsk wrote:
> I have the same comment about the IR checks here.
I think it is better to keep them here unless it creates big overhead which I doubt.


================
Comment at: test/profile/Linux/counter_promo_while.c:41
+  int i = 0;
+  while (i < N) {
+    if (i < n + 1)
----------------
vsk wrote:
> Would placing a nested for loop here increase test coverage?
Probably a little -- as it tests the behavior of promoter with multiple loops in the same nest.


https://reviews.llvm.org/D34085