This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] improve reverse assumption inference
ClosedPublic

Authored by nikic on Aug 28 2017, 8:59 AM.

Details

Summary

Use isGuaranteedToTransferExecutionToSuccessor instead of
isSafeToSpeculativelyExecute when seeing whether we can propagate the
information in an assume backwards in isValidAssumeForContext. The
latter is more general - it also allows arbitrary loads/stores - and is
also the condition we want: if our assume is guaranteed to execute, it
condition not holding would be UB.

Diff Detail

Event Timeline

arielb1 created this revision.Aug 28 2017, 8:59 AM
arielb1 added a reviewer: chandlerc.
hfinkel edited edge metadata.Aug 31 2017, 7:41 AM

This looks good, however, you need to either:

  1. Don't remove isAssumeLikeIntrinsic, but instead, update isGuaranteedToTransferExecutionToSuccessor to call it.
  2. Update isGuaranteedToTransferExecutionToSuccessor to have the complete list from isAssumeLikeIntrinsic.

Right now, isGuaranteedToTransferExecutionToSuccessor only checks:

return CS.onlyReadsMemory() || CS.onlyAccessesArgMemory() ||
       match(I, m_Intrinsic<Intrinsic::assume>());

(At some point, now that we could use post-dominance, you may want to see if it helps. For starters, anything if control dependent on the entry block is guaranteed to execute)

arielb1 updated this revision to Diff 113470.Aug 31 2017, 2:14 PM

add annotations to isGuaranteedToTransferExecutionToSuccessor (they were present in isAssumeLikeIntrinsic, which was *another* orthogonal method that tried to obtain the same result)

Can you please add some test cases for this? From the description, it seems like making a variant of the first test in test/Transforms/InstCombine/assume.ll would work (with some load, etc. that isSafeToSpeculativelyExecute won't handle but`isGuaranteedToTransferExecutionToSuccessor`. Also, adding tests for all of the intrinsics in isAssumeLikeIntrinsic would be really helpful (because at some point we'll need to fix the semantics in isGuaranteedToTransferExecutionToSuccessor and we'd like not to regress).

arielb1 updated this revision to Diff 113697.Sep 3 2017, 7:15 AM

Added test

hfinkel accepted this revision.Sep 3 2017, 7:16 AM

Added test

Thanks, LGTM.

This revision is now accepted and ready to land.Sep 3 2017, 7:16 AM
fhahn added a subscriber: fhahn.Nov 2 2017, 1:02 PM
nikic commandeered this revision.Jul 14 2019, 8:42 AM
nikic added a reviewer: arielb1.
nikic added a subscriber: nikic.

Looks like this never landed...

nikic updated this revision to Diff 209734.Jul 14 2019, 8:44 AM
nikic removed a reviewer: llvm-commits.
nikic set the repository for this revision to rL LLVM.

Rebase.

Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2019, 8:44 AM
lebedev.ri added inline comments.
test/Transforms/InstCombine/inevitable.ll
1 ↗(On Diff #113697)

Precommit using utils/update_test_checks tool?

nikic requested review of this revision.Jul 14 2019, 8:47 AM

In the meantime additional users of isAssumeLikeIntrinsic() have been added, so I've switched this to call isAssumeLikeIntrinsic() from isGuaranteedToTransferExecutionToSuccessor() instead. Does this still look good?

nikic updated this revision to Diff 209735.Jul 14 2019, 9:15 AM

Rebase over baseline tests.

nikic marked 2 inline comments as done.Jul 14 2019, 9:15 AM
nikic added inline comments.
test/Transforms/InstCombine/inevitable.ll
1 ↗(On Diff #113697)

Done!

uenoku added a subscriber: uenoku.Jul 27 2019, 9:18 AM

@uenoku This looks like sth. the Attributor should do as well (at some point), I mean use llvm.assume information

@uenoku This looks like sth. the Attributor should do as well (at some point), I mean use llvm.assume information

It seems interesting.

llvm/lib/Analysis/ValueTracking.cpp
567

MustBeExecutedContextExplorer (in D65186) can simplify this check.

4273

D65455 gives annotation for AssumeLikeInst so you will be able to remove this check.

nikic updated this revision to Diff 212888.Aug 1 2019, 12:56 PM
nikic marked an inline comment as done.

Remove isAssumeLikeIntrinsic handling, this is now handled by willreturn attributes on intrinsics.

nikic edited reviewers, added: spatel, lebedev.ri; removed: eddyb, nagisa.Aug 7 2019, 8:02 AM
nikic marked an inline comment as done.

I suspect that if this is rewritten with MustBeExecutedContextExplorer, @jdoerfert would review.

jdoerfert accepted this revision.Aug 12 2019, 1:02 PM

I suspect that if this is rewritten with MustBeExecutedContextExplorer, @jdoerfert would review.

That depends on the use case. I think we should use the explorer either way but initially in "block-only" mode, which is what is happening here.
One can then decide if more elaborate modes make sense.

For now, I'd be fine with not using the explorer to not block this. In fact, this looks good to me.

This revision is now accepted and ready to land.Aug 12 2019, 1:02 PM
This revision was automatically updated to reflect the committed changes.