[PATCH] D19488: [CodeGenPrepare] use branch weight metadata to decide if a select should be turned into a branch

[Sanjay Patel via llvm-commits]

spatel marked 6 inline comments as done.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4551
@@ +4550,3 @@
+
+  // If metadata tells us that the select condition is obviously predictable,
+  // then we want to replace the select with a branch.
----------------
spatel wrote:
> Good point - we use it in splitBranchCondition() but forgot to update this path.
> I'll fix that ahead of this patch.
rL267504

================
Comment at: lib/IR/Metadata.cpp:1263
@@ +1262,3 @@
+
+  if (auto *ProfileDataName = dyn_cast<MDString>(ProfileData->getOperand(0)))
+    if (!ProfileDataName->getString().equals("branch_weights"))
----------------
spatel wrote:
> Sure - I'll check-in the extra check + comment + variable name changes and update this.
rL267491

================
Comment at: test/CodeGen/X86/cmov-into-branch.ll:118
@@ -96,2 +117,3 @@
+
 !0 = !{!"branch_weights", i32 1, i32 99}
 !1 = !{!"branch_weights", i32 1, i32 100}
----------------
davidxl wrote:
> spatel wrote:
> > Hmm - do you want to check for something different than 'weighted_select1()' above?
> Ok - I mis-read the case -- what needs to be covered is already covered.  A minor suggestion -- the tests (select2, select3) are intended to test that meta data can drive decision to create branch -- but not necessarily to test the actual default 99%,1% threshold. For this reason, it might be better to make !1 and !2 prof data more extreme so that the tests are more robust.
I actually want these tests to perform double-duty:
1. They check that profile data is driving the codegen.
2. They check the exact boundary where that decision changes.

If we make the prof data more extreme, then someone can come by in the future and change the default threshold without breaking any tests. I don't think we want that. We want a change in the default value to be justified and testable.


http://reviews.llvm.org/D19488