[PATCH] D19299: lower __builtin_expect() directly to prof metadata instead of LLVM intrinsic

[David Li via cfe-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:636
@@ -640,1 +635,3 @@
+  case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect: {
+    // Always return the first argument. LLVM does not handle these builtins.
----------------
Can this be reordered with unpredicatle case so that it can handle arg 1 and fall through?

================
Comment at: lib/CodeGen/CGStmt.cpp:1550
@@ -1549,3 +1549,3 @@
 
   // If the switch has a condition wrapped by __builtin_unpredictable,
   // create metadata that specifies that the switch is unpredictable.
----------------
update the comment here.

================
Comment at: lib/CodeGen/CGStmt.cpp:1562
@@ +1561,3 @@
+
+        // FIXME: builtin_expect should use the same metadata type as
+        // builtin_unpredictable and be handled above. For now, we're mimicking
----------------
I am not sure about this. builtin_expect can be used to do general value profiling annotation (single value). For instance,

if (builtin_expect(a, 20) > 10) {
}

should have same effect as

if (builtin_expect(a > 10), true) {

}

The above can be handled by gcc, but not LLVM.


It can be useful for switch case annotation:

switch (__builtin_expect(v, 20) ) {
 case 10: ...
 case 20: ...
 ...
}
where compiler can do switch peeling.

Longer term, I am thinking extending builtin_expect to take list of values with probabilities and predicatibiity hint.

================
Comment at: lib/CodeGen/CGStmt.cpp:1566
@@ +1565,3 @@
+
+        // HACK: Hardcode the taken/not-taken weights based on the existing LLVM
+        // default values. This code is expected to be very temporary. Once we
----------------
Unpredicable meta data is probably not suitable for switch annotation. builtin_expect can be used to specify one case that is more likely to be taken thus helping switch lowering decision (not used in the cases such as if conversion).

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1312
@@ -1311,3 +1311,3 @@
 
   // If the branch has a condition wrapped by __builtin_unpredictable,
   // create metadata that specifies that the branch is unpredictable.
----------------
Update comment here.

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1325
@@ +1324,3 @@
+
+        // FIXME: builtin_expect should use the same metadata type as
+        // builtin_unpredictable and be handled above. For now, we're mimicking
----------------
I suggest removing these comments.


http://reviews.llvm.org/D19299