[ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. #77782

AlfieRichardsArm · 2024-01-11T15:17:15Z

This avoids possible undefined behavior using the same register for Rm and Rda.

Additionally adds a check in MC to produce an error upon parsing this case.

github-actions · 2024-01-11T15:17:32Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-11T15:17:59Z

@llvm/pr-subscribers-mc
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-arm

Author: None (AlfieRichardsArm)

Changes

This avoids possible undefined behavior using the same register for Rm and Rda.

Additionally adds a check in MC to produce an error upon parsing this case.

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

4 Files Affected:

(modified) llvm/lib/Target/ARM/ARMInstrMVE.td (+1-1)
(modified) llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (+8)
(added) llvm/test/CodeGen/AArch64/sqrshr-uqrshl-unpredictable.ll (+21)
(added) llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s (+6)

diff --git a/llvm/lib/Target/ARM/ARMInstrMVE.td b/llvm/lib/Target/ARM/ARMInstrMVE.td
index fa25c571a9bd5e..12c3968b9cecea 100644
--- a/llvm/lib/Target/ARM/ARMInstrMVE.td
+++ b/llvm/lib/Target/ARM/ARMInstrMVE.td
@@ -478,7 +478,7 @@ def MVE_URSHR : MVE_ScalarShiftSRegImm<"urshr", 0b01>;
 
 class MVE_ScalarShiftSRegReg<string iname, bits<2> op5_4>
   : MVE_ScalarShiftSingleReg<iname, (ins rGPR:$RdaSrc, rGPR:$Rm),
-                     "$RdaSrc, $Rm", "$RdaDest = $RdaSrc",
+                     "$RdaSrc, $Rm", "@earlyclobber $RdaDest,$RdaDest = $RdaSrc",
                      [(set rGPR:$RdaDest,
                          (i32 (!cast<Intrinsic>("int_arm_mve_" # iname)
                                    (i32 rGPR:$RdaSrc), (i32 rGPR:$Rm))))]> {
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 18dccb26b87769..7414b5307f704c 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -8308,6 +8308,14 @@ bool ARMAsmParser::validateInstruction(MCInst &Inst,
       return Error (Operands[3]->getStartLoc(), "Q-register indexes must be 2 and 0 or 3 and 1");
     break;
   }
+  case ARM::MVE_SQRSHR:
+  case ARM::MVE_UQRSHL: {
+    if (Operands[2]->getReg() == Operands[3]->getReg()) {
+      return Error (Operands[2]->getStartLoc(),
+                    "Rda register and Rm register can't be identical");
+    }
+    break;
+  }
   case ARM::UMAAL:
   case ARM::UMLAL:
   case ARM::UMULL:
diff --git a/llvm/test/CodeGen/AArch64/sqrshr-uqrshl-unpredictable.ll b/llvm/test/CodeGen/AArch64/sqrshr-uqrshl-unpredictable.ll
new file mode 100644
index 00000000000000..d393a474dfa63a
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/sqrshr-uqrshl-unpredictable.ll
@@ -0,0 +1,21 @@
+; RUN: llc -mtriple armv8.1m.main -mattr=+mve %s -o - | FileCheck %s
+
+; Check that we don't create an unpredictable sqrshr or uqrshl instruction,
+; e.g. sqrshr r0, r0
+
+declare i32 @llvm.arm.mve.sqrshr(i32, i32) #1
+declare i32 @llvm.arm.mve.uqrshl(i32, i32) #1
+
+define i32 @sqrshr() #0 {
+; CHECK-LABEL: sqrshr
+; CHECK-NOT: sqrshr  r[[REG:[0-9]+]], r[[REG]]
+  %1 = tail call i32 @llvm.arm.mve.sqrshr(i32 1, i32 1)
+  ret i32 %1
+}
+
+define i32 @uqrshl() #0 {
+; CHECK-LABEL: uqrshl
+; CHECK-NOT: uqrshl  r[[REG:[0-9]+]], r[[REG]]
+  %1 = tail call i32 @llvm.arm.mve.uqrshl(i32 1, i32 1)
+  ret i32 %1
+}
diff --git a/llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s b/llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s
new file mode 100644
index 00000000000000..3b78b826538f77
--- /dev/null
+++ b/llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s
@@ -0,0 +1,6 @@
+@ RUN: not llvm-mc -triple armv8.1m.main -mattr=+mve < %s 2>&1 | FileCheck %s
+
+  sqrshr    r1, r1
+@ CHECK: [[@LINE-1]]:{{[0-9]+}}: error: Rda register and Rm register can't be identical
+  uqrshl   r1, r1
+@ CHECK: [[@LINE-1]]:{{[0-9]+}}: error: Rda register and Rm register can't be identical

github-actions · 2024-01-11T15:20:22Z

✅ With the latest revision this PR passed the C/C++ code formatter.

This avoids possible undefined behaviour using the same register for Rm and Rda. Additionally adds a check in MC to produce an error upon parsing this case.

jthackray

LGTM.

AlfieRichardsArm · 2024-01-11T15:33:16Z

@jthackray Updated this after I worked out how to run clang-format properly

jthackray

LGTM

davemgreen · 2024-01-11T16:19:39Z

llvm/test/CodeGen/AArch64/sqrshr-uqrshl-unpredictable.ll

@@ -0,0 +1,21 @@
+; RUN: llc -mtriple armv8.1m.main -mattr=+mve %s -o - | FileCheck %s


This test should be under CodeGen/Arm or CodeGen/Thumb2, as it's not AArch64.

I would also personally use the update_llc_test_checks script to generate the check lines.

Thank you, I have moved the test

Unfortunately I couldn't get the update_llc_test_checks took to work, I ran into the same issue as #66409

davemgreen

That sounds OK. The test is probably good as-is. If you can move the other test then this LGTM too. Thanks

davemgreen · 2024-01-11T18:08:19Z

llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s

@@ -0,0 +1,6 @@
+@ RUN: not llvm-mc -triple armv8.1m.main -mattr=+mve < %s 2>&1 | FileCheck %s


This test can be moved too :)

Oh I just checked this and seemingly my eyes skipped right over it. Thank you!

…st/MC/ARM

davemgreen

Thanks. LGTM

…vm#77782) This avoids possible undefined behavior using the same register for Rm and Rda. Additionally adds a check in MC to produce an error upon parsing this case.

llvmbot added backend:ARM backend:AArch64 mc Machine (object) code labels Jan 11, 2024

jthackray self-requested a review January 11, 2024 15:19

[Aarch64] Add missing earlyclobber to sqrshr and uqrshl instructions.

60f7222

This avoids possible undefined behaviour using the same register for Rm and Rda. Additionally adds a check in MC to produce an error upon parsing this case.

jthackray approved these changes Jan 11, 2024

View reviewed changes

AlfieRichardsArm force-pushed the earlyclobber_sqrshr_uqrshl branch from b7a540e to 60f7222 Compare January 11, 2024 15:32

AlfieRichardsArm requested a review from jthackray January 11, 2024 15:45

jthackray approved these changes Jan 11, 2024

View reviewed changes

jthackray requested a review from ostannard January 11, 2024 15:51

davemgreen reviewed Jan 11, 2024

View reviewed changes

Move sqrshr-uqrshl-unpredictable.ll test to llvm/test/CodeGen/ARM

42889a0

AlfieRichardsArm requested a review from davemgreen January 11, 2024 18:04

davemgreen reviewed Jan 11, 2024

View reviewed changes

Move llvm/test/MC/AArch64/mve-sqrshr-uqrshl-earlyclobber.s to llvm/te…

ba64be9

…st/MC/ARM

AlfieRichardsArm requested a review from davemgreen January 11, 2024 18:16

davemgreen approved these changes Jan 11, 2024

View reviewed changes

AlfieRichardsArm changed the title ~~[Aarch64] Add missing earlyclobber to sqrshr and uqrshl instructions.~~ [ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. Jan 12, 2024

ostannard merged commit 60c7757 into llvm:main Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. #77782

[ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. #77782

Uh oh!

AlfieRichardsArm commented Jan 11, 2024

Uh oh!

github-actions bot commented Jan 11, 2024

Uh oh!

llvmbot commented Jan 11, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jan 11, 2024 •

edited

Loading

Uh oh!

jthackray left a comment

Uh oh!

AlfieRichardsArm commented Jan 11, 2024

Uh oh!

jthackray left a comment

Uh oh!

davemgreen Jan 11, 2024

Uh oh!

AlfieRichardsArm Jan 11, 2024

Uh oh!

davemgreen left a comment

Uh oh!

davemgreen Jan 11, 2024

Uh oh!

AlfieRichardsArm Jan 11, 2024

Uh oh!

davemgreen left a comment

Uh oh!

Uh oh!

		@@ -0,0 +1,21 @@
		; RUN: llc -mtriple armv8.1m.main -mattr=+mve %s -o - \| FileCheck %s

		@@ -0,0 +1,6 @@
		@ RUN: not llvm-mc -triple armv8.1m.main -mattr=+mve < %s 2>&1 \| FileCheck %s

[ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. #77782

[ARM] Add missing earlyclobber to sqrshr and uqrshl instructions. #77782

Uh oh!

Conversation

AlfieRichardsArm commented Jan 11, 2024

Uh oh!

github-actions bot commented Jan 11, 2024

Uh oh!

llvmbot commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jthackray left a comment

Choose a reason for hiding this comment

Uh oh!

AlfieRichardsArm commented Jan 11, 2024

Uh oh!

jthackray left a comment

Choose a reason for hiding this comment

Uh oh!

davemgreen Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

AlfieRichardsArm Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

davemgreen left a comment

Choose a reason for hiding this comment

Uh oh!

davemgreen Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

AlfieRichardsArm Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

davemgreen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading