This is an archive of the discontinued LLVM Phabricator instance.

SpeculativeExecution: Fix for logic change introduced in D81730.
ClosedPublic

Authored by dfukalov on Jun 29 2020, 5:42 AM.

Details

Summary

The test case started to hoist bitcasts to upper BB after D81730.
Reverted unintentional logic change. Some instructions may have zero cost but
will not be hoisted by different limitation so should be counted for threshold.

Diff Detail

Event Timeline

dfukalov created this revision.Jun 29 2020, 5:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:42 AM

Seems reasonable to me based on the original intention here.

arsenm added inline comments.Jun 29 2020, 1:12 PM
llvm/test/Transforms/SpeculativeExecution/PR46267.ll
6

I don't understand what this is checking. Can you ad a comment and/or rename the function?

aprantl added inline comments.Jun 29 2020, 2:54 PM
llvm/test/Transforms/SpeculativeExecution/PR46267.ll
10

Yeah I would have expected a hoisted debug intrinsic somewhere in the test?

dfukalov updated this revision to Diff 274454.Jun 30 2020, 6:41 AM

Test updated

dfukalov marked 2 inline comments as done.Jun 30 2020, 6:49 AM
dfukalov added inline comments.
llvm/test/Transforms/SpeculativeExecution/PR46267.ll
6

This is a reduced buildbot testcase started to fail after D81730. The change started to hoist two bitcasts to upper BB although it was not intended in fix for PR46267.

This test checks that bitscasts stayed in their BB.

dfukalov marked an inline comment as done.Jul 7 2020, 10:25 AM

Ping.

aprantl accepted this revision.Jul 8 2020, 3:40 PM
This revision is now accepted and ready to land.Jul 8 2020, 3:40 PM
This revision was automatically updated to reflect the committed changes.