This is an archive of the discontinued LLVM Phabricator instance.

Strip metadata when speculatively hoisting instructions
ClosedPublic

Authored by igor-laevsky on Nov 5 2015, 2:31 PM.

Details

Summary

This is fix for PR24059.

When we are hoisting instruction above some condition it may turn out that metadata on this instruction was control dependant on the condition. So metadata on the hoisted instruction becomes invalid and we need to drop it. See the bug and test cases for the llvm-ir examples.

This patch should cover most obvious places of speculative execution (which I have found by greping isSafeToSpeculativelyExecute). I think there is more cases but at least it covers the severe ones.

Diff Detail

Repository
rL LLVM

Event Timeline

igor-laevsky retitled this revision from to Strip metadata when speculatively hoisting instructions.
igor-laevsky updated this object.
igor-laevsky added reviewers: sanjoy, chandlerc, reames.
igor-laevsky set the repository for this revision to rL LLVM.
igor-laevsky added a subscriber: llvm-commits.
sanjoy accepted this revision.Nov 5 2015, 4:02 PM
sanjoy edited edge metadata.

With the nits addressed, this LGTM.

lib/Analysis/LoopInfo.cpp
125

Nit: "dependent"

126

Nit: Conservatively strip

Fix these spelling errors elsewhere too.

lib/Transforms/Scalar/LICM.cpp
675

This is clearly a step in the right direction.

However, I think we can be more precise than this: (unless I'm missing something) if we justified hoisting the operation using isGuaranteedToExecute and not via isSafeToSpeculativelyExecute, I think we can keep the metadata. Perhaps something like that could be done in a separate patch?

This revision is now accepted and ready to land.Nov 5 2015, 4:02 PM
This revision was automatically updated to reflect the committed changes.
igor-laevsky added inline comments.Nov 10 2015, 6:15 AM
lib/Transforms/Scalar/LICM.cpp
675

I think you are right. I'll probably try to come up with something about this.