Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TableGen] Ignore inaccessible memory when checking pattern flags #90061

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Apr 25, 2024

In the AMDGPU backend we have some cases where we'd like to mark an
intrinsic as IntrInaccessibleMemOnly to model dependencies, but the
corresponding MachineInstrs use uses/defs of a special physical register
to express the same thing. In this case TableGen would complain:

Pattern doesn't match mayLoad/mayStore = 0

but the error is not useful.

In the AMDGPU backend we have some cases where we'd like to mark an
intrinsic as IntrInaccessibleMemOnly to model dependencies, but the
corresponding MachineInstrs use uses/defs of a special physical register
to express the same thing. In this case TableGen would complain:

  Pattern doesn't match mayLoad/mayStore = 0

but the error is not useful.
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 25, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

Changes

In the AMDGPU backend we have some cases where we'd like to mark an
intrinsic as IntrInaccessibleMemOnly to model dependencies, but the
corresponding MachineInstrs use uses/defs of a special physical register
to express the same thing. In this case TableGen would complain:

Pattern doesn't match mayLoad/mayStore = 0

but the error is not useful.


Full diff: https://github.com/llvm/llvm-project/pull/90061.diff

1 Files Affected:

  • (modified) llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp (+9-1)
diff --git a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
index 7a5d2be3ae95b2..e0144a9d8ed2a7 100644
--- a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
@@ -3616,7 +3616,15 @@ class InstAnalyzer {
       hasChain = true;
 
     if (const CodeGenIntrinsic *IntInfo = N.getIntrinsicInfo(CDP)) {
-      ModRefInfo MR = IntInfo->ME.getModRef();
+      // Ignore reads/writes to inaccessible memory. These should not imply
+      // mayLoad/mayStore on the instruction because they are often used to
+      // model dependencies that Machine IR expresses as uses/defs of a
+      // special physical register.
+      ModRefInfo MR = ModRefInfo::NoModRef;
+      for (MemoryEffects::Location Loc : MemoryEffects::locations()) {
+        if (Loc != MemoryEffects::Location::InaccessibleMem)
+          MR |= IntInfo->ME.getModRef();
+      }
       // If this is an intrinsic, analyze it.
       if (isRefSet(MR))
         mayLoad = true; // These may load memory.

@jayfoad
Copy link
Contributor Author

jayfoad commented Apr 25, 2024

This would help with #89612.

Copy link
Collaborator

@rampitec rampitec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jayfoad jayfoad merged commit 6578356 into llvm:main Apr 26, 2024
5 of 6 checks passed
@jayfoad jayfoad deleted the tablegen-ignore-inaccessible branch April 26, 2024 09:28
ModRefInfo MR = ModRefInfo::NoModRef;
for (MemoryEffects::Location Loc : MemoryEffects::locations()) {
if (Loc != MemoryEffects::Location::InaccessibleMem)
MR |= IntInfo->ME.getModRef();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly typo here. It should have been:

Suggested change
MR |= IntInfo->ME.getModRef();
MR |= IntInfo->ME.getModRef(Loc);

Because of this the patch had no effect, and if I fix the typo then it causes some build failures.

I will revert and rethink.

jayfoad added a commit that referenced this pull request Apr 26, 2024
…lags (#90061)"

This reverts commit 6578356.

The patch had no effect due to a silly mistake and fixing the mistake
causes other problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants