[PATCH] D23727: [Profile] SelectInst instrumentation Support in IR-PGO

[Vedant Kumar via llvm-commits]

vsk added a comment.

This is an interesting patch! I'm excited to see how the new information will be used.

I have two notes about documentation: 1) this patch needs to update docs/LangRef.rst, and 2) docs/BranchWeightMetadata.rst claims that SelectInst is not allowed to have branch_weight metadata -- so either the docs need fixing or the Verifier needs fixing (or both!). I can help with that if you like.


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:240
@@ -223,3 +239,3 @@
   Value *Count = Builder.CreateLoad(Addr, "pgocount");
-  Count = Builder.CreateAdd(Count, Builder.getInt64(1));
+  Count = Builder.CreateAdd(Count, getIncrementStep(Inc, Builder.getInt64(1)));
   Inc->replaceAllUsesWith(Builder.CreateStore(Count, Addr));
----------------
Why not pass the IRBuilder in to avoid creating the DefaultStep until it's needed?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:183
@@ +182,3 @@
+  // Visit \p SI instruction and perform tasks according to visit mode.
+  void visitSelectInst(SelectInst &SI);
+  unsigned size() const { return NSIs; }
----------------
override?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:184
@@ +183,3 @@
+  void visitSelectInst(SelectInst &SI);
+  unsigned size() const { return NSIs; }
+};
----------------
'size' is an odd name for a method in an InstVisitor. How about 'getNumSelectInsts'?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:978
@@ +977,3 @@
+  }
+}
+
----------------
My impression is that this could be better structured / easier to read as two different InstVisitors. Both visitors would maintain less state than the current SelectInstVisitor, and their constructors should obviate the need SetCounterIndex and SetCounterUse.

================
Comment at: test/Transforms/PGOProfile/select1.ll:2
@@ +1,3 @@
+; RUN: opt < %s -pgo-instr-gen -pgo-instr-select=true -S | FileCheck %s --check-prefix=GEN
+; RUN: opt < %s -passes=pgo-instr-gen -pgo-instr-select=true -S | FileCheck %s --check-prefix=GEN
+; RUN: llvm-profdata merge %S/Inputs/select1.proftext -o %t.profdata
----------------
Can you check that the increment-step instruction disappears with -pgo-instr-select=false?

================
Comment at: test/Transforms/PGOProfile/select1.ll:17
@@ +16,3 @@
+;GEN: [[STEP:.*]] = zext i1 %cmp to i64
+;GEN: call void @llvm.instrprof.increment.step({{.*}} i32 3, i32 2, i64 %0)
+  %s = select i1 %cmp, i32 %add, i32 0
----------------
'i64 [[STEP]]' instead of 'i64 %0'?


https://reviews.llvm.org/D23727