Page MenuHomePhabricator

Preserve loop metadata when folding branches to a common destination
ClosedPublic

Authored by mkuper on Dec 15 2016, 3:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 81677.Dec 15 2016, 3:33 PM
mkuper retitled this revision from to Preserve loop metadata when folding branches to a common destination .
mkuper updated this object.
mkuper added reviewers: hfinkel, fhahn, danielcdh.
mkuper added a subscriber: llvm-commits.
danielcdh edited edge metadata.Dec 15 2016, 3:57 PM

When/how was MD_loop set/used? Is it related to profile?

When/how was MD_loop set/used? Is it related to profile?

No, it's not related to profile information.
It's used to do things like explicitly enable or disable vectorization, unrolling, etc. I added you to the review because this is similar in spirit to what you did for the profile information.

fhahn edited edge metadata.Dec 15 2016, 4:10 PM

Moving the loop metadata to the new latch looks good to me and I think it's a great idea to preserve it as best as we can. I'm reluctant to accept the revision only because I'm fairly new to LLVM and my experience with SimplifyCFG.cpp is rather limited.

@danielcdh the language ref provides a good overview over the information contained in MD_Loop (http://llvm.org/docs/LangRef.html#llvm-loop)

Thanks for the clarification and Thanks Florian for the pointer to the doc.

The patch LGTM, but I think Hal would be more qualified than I do to approve this.

Optional: while you are at it, it'll be very useful to handle the MD_prof here for branch probability. (e.g. when PBI has no MD_prof while BI has, or like the case described in TODO comment).

Thanks,
Dehao

lib/Transforms/Utils/SimplifyCFG.cpp
2795 ↗(On Diff #81677)

Maybe make it explicit in the comment that "BI was a loop latch" means "BI has MD_loop" Metadata.

Optional: while you are at it, it'll be very useful to handle the MD_prof here for branch probability. (e.g. when PBI has no MD_prof while BI has, or like the case described in TODO comment).

I'm a bit confused, I thought merging MD_prof is what the code directly above this does.

lib/Transforms/Utils/SimplifyCFG.cpp
2795 ↗(On Diff #81677)

Right, what I was trying to say was that MD_loop is expected to live on loop latches, so that's when BI could have MD_loop.
I'll rephrase, thanks.

Optional: while you are at it, it'll be very useful to handle the MD_prof here for branch probability. (e.g. when PBI has no MD_prof while BI has, or like the case described in TODO comment).

I'm a bit confused, I thought merging MD_prof is what the code directly above this does.

You are right, it's already done, I did not look above.

hfinkel accepted this revision.Dec 16 2016, 8:33 AM
hfinkel edited edge metadata.

Thanks for the clarification and Thanks Florian for the pointer to the doc.

The patch LGTM, but I think Hal would be more qualified than I do to approve this.

LGTM too.

This revision is now accepted and ready to land.Dec 16 2016, 8:33 AM
This revision was automatically updated to reflect the committed changes.