[PATCH] D74809: Include static prof data when collecting loop BBs

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

I'm in no way qualified to be reviewing changes to something with as important implications as machine block placement, but from the test modifications, it doesn't seem too invasive a change.  Posting mostly notes for other reviewers to check.



================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2517
+        const BasicBlock *BB = MBB->getBasicBlock();
+        return BB && BB->getTerminator()->hasMetadata(LLVMContext::MD_prof);
+      })) {
----------------
I really wish this was a method on `MachineBlockPlacement` or `MachineLoop` rather than a lambda in an `if` statement.


================
Comment at: llvm/test/CodeGen/Hexagon/prof-early-if.ll:5
 ; CHECK-NOT: if{{.*}}memd
+; CHECK: b5
 
----------------
LGTM; b5 is only branched to from b4, but b4's has higher branch weight to b2. Though it would be nice to just list all the blocks in order; otherwise I'm just guessing where the rest land.


================
Comment at: llvm/test/CodeGen/X86/block-placement-2.ll:13
+; CHECK:       %if.else
+; CHECK:       %if.then23
+; CHECK:       ret
----------------
probably should specify where `if.end.i.i` gets placed, too, since it also has profile metadata.


================
Comment at: llvm/test/CodeGen/X86/block-placement.ll:1507
 ; CHECK: %.stop
+; CHECK: %.slow
 .entry:
----------------
LGTM; backedge is very likely to go to header. middle is unlikely to go to slow


================
Comment at: llvm/test/CodeGen/X86/move_latch_to_loop_top.ll:177
 ;CHECK:       %exit
+;CHECK:       %false
 define i32 @test4(i32 %t, i32* %p) {
----------------
LGTM; %latch has a 50-50 chance of branching either to %exit or %header. `!3` seems unused.


================
Comment at: llvm/test/CodeGen/X86/ragreedy-bug.ll:35
 ; CHECK-NEXT: movzbl
-; CHECK-NEXT: testl
-; CHECK-NEXT: jne
+; CHECK-NEXT: jmp
 
----------------
this change is curious; I'd have expected `je`'s to flip to `jne`'s or vice versa, but not unconditional jumps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74809