This is an archive of the discontinued LLVM Phabricator instance.

[IR] Mark `llvm.assume` as `memory(inaccessiblemem: write)`
ClosedPublic

Authored by jdoerfert on Jul 27 2023, 12:23 PM.

Details

Summary

It was inaccessiblemem: readwrite before, no need for the read.
No real benefit is expected but it can help debugging and other efforts.

Diff Detail

Event Timeline

jdoerfert created this revision.Jul 27 2023, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 12:23 PM
jdoerfert requested review of this revision.Jul 27 2023, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2023, 12:23 PM

For some reason, the vectorizer is impacted by write only on the assume:
https://godbolt.org/z/MM5W9rE3M
I doubt this is good.

The problem is that we only check intrinsics with read effects, so this code won't be executed with this patch (except if we hoist it out of the conditional):

// Many math library functions read the rounding mode. We will only
// vectorize a loop if it contains known function calls that don't set
// the flag. Therefore, it is safe to ignore this read from memory.
auto *Call = dyn_cast<CallInst>(&I);
if (Call && getVectorIntrinsicIDForCall(Call, TLI))
   continue;

Assuming that assume can only be used for path-/flow- insensitive invariants (that hold for the whole program), this change is correct. The implication is that the order of assumes doesn't matter.

Alas, with the previous spec, it was already possible to hoist assumes out of branches, so it implied it couldn't be used to assume facts just in a specific branch.
I need to change Alive2's semantics to make sure that LLVM is using assume in this way only.

nikic added a comment.Jul 28 2023, 2:45 AM

Assuming that assume can only be used for path-/flow- insensitive invariants (that hold for the whole program), this change is correct. The implication is that the order of assumes doesn't matter.

Alas, with the previous spec, it was already possible to hoist assumes out of branches, so it implied it couldn't be used to assume facts just in a specific branch.
I need to change Alive2's semantics to make sure that LLVM is using assume in this way only.

Assume definitely needs to be flow sensitive. But I don't understand why this change would remove the flow sensitivity. Assume is still non-speculatable, so it cannot be moved outside of branches.

nlopes accepted this revision.Jul 28 2023, 3:32 AM

Assuming that assume can only be used for path-/flow- insensitive invariants (that hold for the whole program), this change is correct. The implication is that the order of assumes doesn't matter.

Alas, with the previous spec, it was already possible to hoist assumes out of branches, so it implied it couldn't be used to assume facts just in a specific branch.
I need to change Alive2's semantics to make sure that LLVM is using assume in this way only.

Assume definitely needs to be flow sensitive. But I don't understand why this change would remove the flow sensitivity. Assume is still non-speculatable, so it cannot be moved outside of branches.

Ah, never mind, I don't know what I was thinking when I wrote that comment 😅 I forgot for a second that assume is a function call in LLVM IR.
Sorry, it's all good.

This revision is now accepted and ready to land.Jul 28 2023, 3:32 AM
This revision was landed with ongoing or failed builds.Jul 31 2023, 1:46 PM
This revision was automatically updated to reflect the committed changes.