[llvm-commits] Built-in "expect" support

[Jakub Staszak]

On Jul 5, 2011, at 9:15 AM, Bob Wilson wrote:

> 
> On Jul 2, 2011, at 6:00 PM, Jakub Staszak wrote:
> 
>> Hello,
>> 
>> This patch introduces built-in support. It uses llvm.expect intrinsics to create "branch_weights" metadata if run with -md-expect or with -O.
>> 
>> Comments are welcome.
> 
> 
> I think the new "expect" intrinsic should be overloaded for all integer types, instead of being forced to i64 only.  Although it should work OK without the overloading, I can imagine cases where the extra sign/zero-extends and truncates for i64 could change the way the code is optimized.  Overloading it just seems like a better fit for how this intrinsic is used.
> 
I used llvm_anyint_ty. It seems to work.

> Unless I missed something, this is your first patch using the "block_weights" metadata, and you haven't yet described the intended usage or format of that metadata.  You'll need to provide those details and some documentation.  For now, let's proceed with the understanding that everything about "block_weights" metadata is tentative and subject to change.
> 
I'm working on the documentation right now.

> The "expect" intrinsic should have the IntrNoMem property, not IntrReadArgMem.
> 
Oh, sure.

> I think your new pass should run after early CSE.  One of the reasons we decided to implement builtin_expect this way is to allow some simple propagation of builtin_expect calls that are not directly inside conditionals.  You haven't implemented that yet, but early CSE should help when the time comes.
> 
OK.

> The long methods in your MetadataExpect pass should be defined outside the class.
> 
Done.

> The arbitrary constants "4" and "64" for likely and unlikely branch weights should not be used directly.  These should be symbolic constants defined in the MetadataExpect class.  Once we have more code using this, we may want to adjust those values.
> 
Done.

> The "MetadataExpect" and "md-expect" names are not very descriptive of what the transform pass does, which is to convert expect intrinsics to block_weights metadata.  I'll see if I can think of better alternatives -- let me know if you have ideas.
> 
LowerExpectIntrinsics?
ExpectToBlockWeights?
{Lower/Transform}ExpectsToBlockWeights?

> Please add a comment for your change to IntrinsicLowering.cpp
Done.

- Kuba