-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[AArch64] Mark Armv8.4-a LDAPUR* instructions as mayLoad #171142
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
base: main
Are you sure you want to change the base?
Conversation
Thanks to @cofibrant for spotting.
|
@llvm/pr-subscribers-backend-aarch64 Author: Cullen Rhodes (c-rhodes) ChangesThanks to @cofibrant for spotting. Full diff: https://github.com/llvm/llvm-project/pull/171142.diff 7 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64InstrFormats.td b/llvm/lib/Target/AArch64/AArch64InstrFormats.td
index 4d2e740779961..6017f5b1abea0 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrFormats.td
+++ b/llvm/lib/Target/AArch64/AArch64InstrFormats.td
@@ -4384,6 +4384,7 @@ class BaseLoadStoreUnscale<bits<2> sz, bit V, bits<2> opc, dag oops, dag iops,
// Armv8.4 LDAPR & STLR with Immediate Offset instruction
multiclass BaseLoadUnscaleV84<string asm, bits<2> sz, bits<2> opc,
DAGOperand regtype > {
+ let mayLoad = 1 in
def i : BaseLoadStoreUnscale<sz, 0, opc, (outs regtype:$Rt),
(ins GPR64sp:$Rn, simm9:$offset), asm, []>,
Sched<[WriteST]> {
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-rcpc-immo-instructions.s
index cd3d7e0bf1b57..fac107ddc5326 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/N3-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/N3-rcpc-immo-instructions.s
index 6faa5e1f4db1b..3442dd4eaff67 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/N3-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/N3-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V1-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V1-rcpc-immo-instructions.s
index 5c9b43a0e5121..76380b499c907 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V1-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V1-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V2-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V2-rcpc-immo-instructions.s
index 71fd689522215..a099fb965a7a0 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V2-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V2-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3-rcpc-immo-instructions.s
index a48978ce8b94d..78542b8b93702 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
diff --git a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3AE-rcpc-immo-instructions.s b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3AE-rcpc-immo-instructions.s
index f801a18bc7a06..1021f80306adb 100644
--- a/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3AE-rcpc-immo-instructions.s
+++ b/llvm/test/tools/llvm-mca/AArch64/Neoverse/V3AE-rcpc-immo-instructions.s
@@ -14,11 +14,11 @@
# CHECK-NEXT: 2 1 0.50 * ldapur x20, [x13]
# CHECK-NEXT: 2 1 0.50 * ldapurb w13, [x17]
# CHECK-NEXT: 2 1 0.50 * ldapurh w3, [x22]
-# CHECK-NEXT: 2 1 0.50 U ldapursb w7, [x8]
-# CHECK-NEXT: 2 1 0.50 U ldapursb x29, [x7]
-# CHECK-NEXT: 2 1 0.50 U ldapursh w17, [x19]
-# CHECK-NEXT: 2 1 0.50 U ldapursh x3, [x3]
-# CHECK-NEXT: 2 1 0.50 U ldapursw x3, [x18]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8]
+# CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19]
+# CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3]
+# CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18]
# CHECK-NEXT: 2 1 0.50 * stlur w3, [x27]
# CHECK-NEXT: 2 1 0.50 * stlur x23, [x25]
# CHECK-NEXT: 2 1 0.50 * stlurb w30, [x17]
|
| # CHECK-NEXT: 2 1 0.50 * U ldapursb w7, [x8] | ||
| # CHECK-NEXT: 2 1 0.50 * U ldapursb x29, [x7] | ||
| # CHECK-NEXT: 2 1 0.50 * U ldapursh w17, [x19] | ||
| # CHECK-NEXT: 2 1 0.50 * U ldapursh x3, [x3] | ||
| # CHECK-NEXT: 2 1 0.50 * U ldapursw x3, [x18] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised these are still marked as 'has side effects'. Could you explain what the side effects are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah tbh im not sure about that either, i can't see why they shouldn't be marked as mayLoad so I suspect that was just an oversight, but I didnt add these instructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context is that I spotted that Apple clang thinks these instructions may load and do not have side effects and I wanted to understand which implementation is correct (if either) and fix accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the semantics of these instructions, but from a quick glance at https://developer.arm.com/documentation/102336/0100/Load-Acquire-and-Store-Release-instructions it looks like they have implicit barrier semantics, so while I can't give a definitive answer I would err more towards them having side-effects than not. dmb instructions do, and these have less constrained but similar semantics it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, thanks!
davemgreen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the other instructions get this from the patterns? They should have atomic side-effects but from the other instructions that I believe that usually comes from the MMO. HasSideEffects should be stronger so wouldn't make it incorrect to keep.
| let mayLoad = 1 in | ||
| def i : BaseLoadStoreUnscale<sz, 0, opc, (outs regtype:$Rt), | ||
| (ins GPR64sp:$Rn, simm9:$offset), asm, []>, | ||
| Sched<[WriteST]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be WriteLD or WriteAtomic. The scheduling info in the tests looks incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! I can post a separate PR.
FWIW I've not been paying close attention to the scheduling info with recent patches tbh, just trying to refactor the tests and increase coverage but i can believe there's many with bad info such as this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
posted a fix for this #171637
ah ok, I didnt realise this until I saw your comment but seems so, when I remove llvm-project/llvm/lib/Target/AArch64/AArch64InstrAtomics.td Lines 602 to 633 in 614fe6d
all of these instructions then just have
ok thanks for the context, I think I can see that now
This isn't set on the instruction so I guess |
|
I should be able to post a patch including some ISel patterns for these instructions in the next couple of days. From these the compiler is able to infer |
ah nice, I'll hold off on landing this for now then |
Thanks to @cofibrant for spotting.