[PATCH] D50591: [PGO] Control Height Reduction

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: test/Transforms/PGOProfile/chr.ll:14
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq i32 [[TMP1]], 3
+; CHECK-NEXT:    br i1 [[TMP2]], label [[BB0:%.*]], label [[ENTRY_SPLIT_NONCHR:%.*]], !prof !15
+; CHECK:       bb0:
----------------
yamauchi wrote:
> davidxl wrote:
> > Can you add check of the meta data itself (for testing of profile meta data merging)?
> There's already a check for !15 (at the end of this line). I added a few more.
that is not what I meant.  I think what is missing is the prof meta data's content. For instance

CHECK-NEXT: br i1 ....... !prof ![[COMBINED_PROF:%.*]]


CHECK:![[COMBINED_PROF] = ...


================
Comment at: test/Transforms/PGOProfile/chr.ll:94
+;     foo()
+;   if ((t0 & 2) != 0)
+;     bar()
----------------
should be == 0


================
Comment at: test/Transforms/PGOProfile/chr.ll:268
+; }
+; t2 = *i
+; if ((t2 & 12) != 0) { // Likely true
----------------
If this can be hoisted above, will the optimization combine (t1 & 3) | (t2 &12) ?


================
Comment at: test/Transforms/PGOProfile/chr.ll:532
+; t0 = *i
+; if ((sum0 & 4) != 0 && (t0 & 11) != 11) { // Likely true
+;   sum = sum0 + 173
----------------
&& should be bitwise &


================
Comment at: test/Transforms/PGOProfile/chr.ll:634
+; j0 = *j
+; if ((j0 & 4) != 0 && (i0 & 10) != 10) { // Likely true
+;   sum = sum0 + 131
----------------
&& -> &


================
Comment at: test/Transforms/PGOProfile/chr.ll:1084
+; two paths and may cause a bad hoisting that hoists it twice and puts a use
+; ahead of its definition.
+; Roughly,
----------------
Not sure if I understand the comments about bad hoisting here. You want to test the bad hoisting does not happen? What does the bad hoisting look like?


================
Comment at: test/Transforms/PGOProfile/chr.ll:1242
+; i0 = *i
+; v2 = (z != 1) ? pred : true  // Likely false
+; if (z == 0 && pred)  // Likely false
----------------
likely true?


================
Comment at: test/Transforms/PGOProfile/chr.ll:1243
+; v2 = (z != 1) ? pred : true  // Likely false
+; if (z == 0 && pred)  // Likely false
+;   foo()
----------------
likely true ?


================
Comment at: test/Transforms/PGOProfile/chr.ll:1254
+; i0 = *i
+; if (z != 1 && (z == 0 && pred)) // First CHR
+;   foo()
----------------
is z != 1 redundant?


================
Comment at: test/Transforms/PGOProfile/chr.ll:1256
+;   foo()
+; // A trivial phi for i0 is inserted here by the first CHR (which gets removed
+; // later) and the subsequent branch condition (for the second CHR) uses it.
----------------
why was it inserted?


================
Comment at: test/Transforms/PGOProfile/chr.ll:1439
+; return v42 + v40
+; ->
+; t0 = *i
----------------
should it be:

; t0 = *i
; v40 = t0 + 44
; if ((t0 & 3) == 3) // Likely true
;   foo()
;   v41 = t0 + 99
;   foo()
; } else {
;   if ((t0 & 1) != 0) // Likely true
;     foo()
;   if ((t0 & 2) != 0) // Likely true
;     v41_nc = t0 + 99
;     foo()
;   }
; }
; v42 = phi (v41, v41_nc, v40)
; return v42 + v40













Repository:
  rL LLVM

https://reviews.llvm.org/D50591