This is an archive of the discontinued LLVM Phabricator instance.

allow branch weight metadata on select instructions (PR26636)
ClosedPublic

Authored by spatel on Mar 13 2016, 10:01 AM.

Details

Summary

As noted in:
https://llvm.org/bugs/show_bug.cgi?id=26636

This doesn't accomplish anything on its own. It's the first step towards preserving and using branch weights with selects.

If this looks ok, the next step would be to make sure we're propagating the info in all of the other places where we create selects (SimplifyCFG, InstCombine, etc). I don't think there's an easy fix to make this happen; we have to look at each transform individually to determine how to correctly propagate the weights.

Along with that step, we need to then use the weights when making subsequent transform decisions such as discussed in http://reviews.llvm.org/D16836.

Diff Detail

Event Timeline

spatel updated this revision to Diff 50553.Mar 13 2016, 10:01 AM
spatel retitled this revision from to allow branch weight metadata on select instructions (PR26636).
spatel updated this object.
spatel added reviewers: hfinkel, bkramer, davidxl.
spatel added a subscriber: llvm-commits.
davidxl added a subscriber: congh.Mar 13 2016, 10:37 AM
davidxl edited edge metadata.Mar 13 2016, 10:45 AM

This definitetly the right thing to do.

include/llvm/IR/IRBuilder.h
1579

The cloneImpl member method of the SelectInstr also needs to be updated to clone the metadata.

spatel added inline comments.Mar 14 2016, 8:26 AM
include/llvm/IR/IRBuilder.h
1579

Please let me know if there's a logic hole, but I think the base Instruction::clone() method handles this, so subclasses don't need to:
http://llvm.org/klaus/llvm/blob/master/lib/IR/Instruction.cpp#L-577

spatel updated this revision to Diff 50604.Mar 14 2016, 9:01 AM
spatel edited edge metadata.

Patch updated:
Rather than creating a new regression test to verify the metadata, just add metadata to the existing test case for the transform.
We're going to need several of these updates, so I don't want to add regression test overhead for duplicate checking of every transform where metadata will be added.

davidxl added inline comments.Mar 14 2016, 9:23 AM
include/llvm/IR/IRBuilder.h
1579

Ok.

Is it possible to add a small test case (or update existing test case) to cover this -- i.e., after cloning (i.e via inlining), the meta data is maintained for select?

spatel added inline comments.Mar 14 2016, 9:39 AM
include/llvm/IR/IRBuilder.h
1579

Sure - that seems possible. But I wonder if we want to have multi-pass regression tests here? Ie, it would require 'opt -inline -simplifycfg'.

I thought we shy away from that form of regression test...although there is precedent in test/Transforms/Inline/invoke_test-2.ll.

davidxl added inline comments.Mar 14 2016, 9:49 AM
include/llvm/IR/IRBuilder.h
1579

Can you feed the inliner with IR that includes profile annotated 'select'? We just need to test the inline instance has 'select' that is also profile annotated.

spatel added inline comments.Mar 14 2016, 12:21 PM
include/llvm/IR/IRBuilder.h
1579

Ah, in that case the test that you would like to see is actually independent of this patch. I'll add it anyway to make sure that I'm understanding. It doesn't look like we even have a test with that profile metadata check for an inlined *branch*.

spatel updated this revision to Diff 50628.Mar 14 2016, 12:22 PM

Patch updated:
Add inliner tests to make sure that cloned selects and branches are preserving profile metadata.

davidxl accepted this revision.Mar 14 2016, 12:33 PM
davidxl edited edge metadata.

lgtm.

include/llvm/IR/IRBuilder.h
1579

yes, this test case is what I am looking for -- I think it should be part of this patch (though the branch instruction meta test testing can be added in a separate patch though I won't mind either way).

I also suggesting changing the file name to 'profile-meta.ll'

This revision is now accepted and ready to land.Mar 14 2016, 12:33 PM
This revision was automatically updated to reflect the committed changes.