Skip to content

Conversation

@xen0n
Copy link
Contributor

@xen0n xen0n commented Oct 31, 2024

Depends on #114508

The LoongArch Reference Manual says that the 3-register atomic memory operations cannot have their rd equal to either rj or rk 1, and both GNU as and LLVM IAS enforce the constraint for non-zero rd. However, currently LoongArch AsmParser is checking for the opcode with a direct numerical comparison on the opcode, which is enum-typed: the fact that all AMO insns have adjacent numerical values is merely a coincidence, and it is better to not rely on the current TableGen implementation behavior.

Instead, start to leverage the target-specific flags field of MCInstrDesc, and record the constraint with TableGen, so we can stop treating the opcode value as number. In doing so, we also have to mark whether the instruction is AMCAS, because the operand index of rj and rk for the AMCAS instructions is different.

While documenting the new flag, it was found that v1.10 of the Manual did not specify the similar constraint for the AMCAS instructions. Experiments were done on a Loongson 3A6000 (LA664 uarch) and it turned out that at least AMCAS will still signal INE with rd == rj. The rd == rk case should be a no-op according to the semantics, but as it is meaningless to perform CAS with the "old value" same as the "new value", it is not worth special-casing. So the current behavior of also enforcing the constraint for AMCAS is kept.

Footnotes

  1. if rd == rj an INE would be signaled; if rd == rk it is UB.

@llvmbot llvmbot added llvm:mc Machine (object) code backend:loongarch labels Oct 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-mc

Author: WÁNG Xuěruì (xen0n)

Changes

The LoongArch Reference Manual says that the 3-register atomic memory operations cannot have their rd equal to either rj or rk 1, and both GNU as and LLVM IAS enforce the constraint. However, currently LoongArch AsmParser is checking for the opcode with a direct numerical comparison on the opcode, which is enum-typed: the fact that all AMO insns have adjacent numerical values is a coincidence, and it is better to not rely on the current TableGen file layout and the particular TableGen implementation behavior.

Instead, start to leverage the target-specific flags field of MCInstrDesc, and record the constraint with TableGen, so we can stop treating the opcode enum as number.

While documenting the new flag, it was found that v1.10 of the Manual did not specify the similar constraint for the AMCAS instructions. Experiments were done on a Loongson 3A6000 (LA664 uarch) and it turned out that at least AMCAS will still signal INE with rd == rj. The rd == rk case should be a no-op according to the semantics, but as it is meaningless to perform CAS with the "old value" same as the "new value", it is not worth special-casing. So the current behavior of also enforcing the constraint for AMCAS is kept.


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

5 Files Affected:

  • (modified) llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp (+3-1)
  • (modified) llvm/lib/Target/LoongArch/LoongArchInstrFormats.td (+5)
  • (modified) llvm/lib/Target/LoongArch/LoongArchInstrInfo.td (+3-1)
  • (modified) llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h (+23)
  • (modified) llvm/test/MC/LoongArch/Basic/Integer/invalid64.s (+11)
diff --git a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
index b4b19caed8999e..a15803355ff2af 100644
--- a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
+++ b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "MCTargetDesc/LoongArchBaseInfo.h"
 #include "MCTargetDesc/LoongArchInstPrinter.h"
 #include "MCTargetDesc/LoongArchMCExpr.h"
 #include "MCTargetDesc/LoongArchMCTargetDesc.h"
@@ -1560,9 +1561,10 @@ bool LoongArchAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
 
 unsigned LoongArchAsmParser::checkTargetMatchPredicate(MCInst &Inst) {
   unsigned Opc = Inst.getOpcode();
+  const MCInstrDesc &MCID = MII.get(Opc);
   switch (Opc) {
   default:
-    if (Opc >= LoongArch::AMADD_D && Opc <= LoongArch::AMXOR_W) {
+    if (LoongArchII::isSubjectToAMORdConstraint(MCID.TSFlags)) {
       MCRegister Rd = Inst.getOperand(0).getReg();
       MCRegister Rk = Inst.getOperand(1).getReg();
       MCRegister Rj = Inst.getOperand(2).getReg();
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td b/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
index 6ffc8823baee01..f49638c0ca93e7 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
@@ -32,6 +32,11 @@ class LAInst<dag outs, dag ins, string opcstr, string opnstr,
   let InOperandList = ins;
   let AsmString = opcstr # "\t" # opnstr;
   let Pattern = pattern;
+
+  // Target-specific instruction info and defaults
+
+  bit IsSubjectToAMORdConstraint = 0;
+  let TSFlags{0} = IsSubjectToAMORdConstraint;
 }
 
 // Pseudo instructions
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
index 671b8cc6ffe1b1..c7d2f0c8efb26f 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
@@ -713,7 +713,9 @@ class STORE_2RI14<bits<32> op>
 let hasSideEffects = 0, mayLoad = 1, mayStore = 1, Constraints = "@earlyclobber $rd" in
 class AM_3R<bits<32> op>
     : Fmt3R<op, (outs GPR:$rd), (ins GPR:$rk, GPRMemAtomic:$rj),
-            "$rd, $rk, $rj">;
+            "$rd, $rk, $rj"> {
+  let IsSubjectToAMORdConstraint = 1;
+}
 
 let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
 class LLBase<bits<32> op>
diff --git a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
index 1a12fb492a60f5..e06138a31bc022 100644
--- a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
+++ b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
@@ -56,6 +56,29 @@ enum {
   MO_DESC_CALL,
   // TODO: Add more flags.
 };
+
+// Target-specific flags of LAInst.
+// All definitions must match LoongArchInstrFormats.td.
+enum {
+  // Whether the instruction's rd is normally required to differ from rj and
+  // rk, in the way the 3-register atomic memory operations behave
+  // (Section 2.2.7.1 and 2.2.7.2, LoongArch Reference Manual Volume 1 v1.10;
+  // while Section 2.2.7.3 lacked similar description for the AMCAS
+  // instructions, at least the INE exception is still signaled on Loongson
+  // 3A6000 when its rd == rj).
+  //
+  // Used for generating diagnostics for assembler input that violate the
+  // constraint. As described on the manual, the covered instructions require
+  // rd != rj && rd != rk to work as intended.
+  IsSubjectToAMORdConstraintShift = 0,
+  IsSubjectToAMORdConstraintMask = 1 << IsSubjectToAMORdConstraintShift,
+};
+
+/// \returns true if this instruction's rd is normally required to differ
+/// from rj and rk, in the way 3-register atomic memory operations behave.
+static inline bool isSubjectToAMORdConstraint(uint64_t TSFlags) {
+  return TSFlags & IsSubjectToAMORdConstraintMask;
+}
 } // end namespace LoongArchII
 
 namespace LoongArchABI {
diff --git a/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s b/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
index 1c1c658ad440f8..0d81ddd6763b56 100644
--- a/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
+++ b/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
@@ -91,3 +91,14 @@ amxor.w $a0, $a1, $a0
 amadd.d $a0, $a1, $a2, $a3
 # CHECK: :[[#@LINE+1]]:24: error: optional integer offset must be 0
 amadd.d $a0, $a1, $a2, 1
+
+## According to experiment results on real LA664 HW, the AMCAS instructions
+## are subject to the same constraint as the other 3-register atomic insns.
+## This is undocumented in v1.10 of the LoongArch Reference Manual.
+
+# CHECK: :[[#@LINE+1]]:10: error: $rd must be different from both $rk and $rj
+amcas.b $a0, $a0, $a0
+# CHECK: :[[#@LINE+1]]:10: error: $rd must be different from both $rk and $rj
+amcas.h $a0, $a0, $a1
+# CHECK: :[[#@LINE+1]]:13: error: $rd must be different from both $rk and $rj
+amcas_db.w $a0, $a1, $a0

Footnotes

  1. if rd == rj an INE would be signaled; if rd == rk it is UB.

@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-backend-loongarch

Author: WÁNG Xuěruì (xen0n)

Changes

The LoongArch Reference Manual says that the 3-register atomic memory operations cannot have their rd equal to either rj or rk 1, and both GNU as and LLVM IAS enforce the constraint. However, currently LoongArch AsmParser is checking for the opcode with a direct numerical comparison on the opcode, which is enum-typed: the fact that all AMO insns have adjacent numerical values is a coincidence, and it is better to not rely on the current TableGen file layout and the particular TableGen implementation behavior.

Instead, start to leverage the target-specific flags field of MCInstrDesc, and record the constraint with TableGen, so we can stop treating the opcode enum as number.

While documenting the new flag, it was found that v1.10 of the Manual did not specify the similar constraint for the AMCAS instructions. Experiments were done on a Loongson 3A6000 (LA664 uarch) and it turned out that at least AMCAS will still signal INE with rd == rj. The rd == rk case should be a no-op according to the semantics, but as it is meaningless to perform CAS with the "old value" same as the "new value", it is not worth special-casing. So the current behavior of also enforcing the constraint for AMCAS is kept.


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

5 Files Affected:

  • (modified) llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp (+3-1)
  • (modified) llvm/lib/Target/LoongArch/LoongArchInstrFormats.td (+5)
  • (modified) llvm/lib/Target/LoongArch/LoongArchInstrInfo.td (+3-1)
  • (modified) llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h (+23)
  • (modified) llvm/test/MC/LoongArch/Basic/Integer/invalid64.s (+11)
diff --git a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
index b4b19caed8999e..a15803355ff2af 100644
--- a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
+++ b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "MCTargetDesc/LoongArchBaseInfo.h"
 #include "MCTargetDesc/LoongArchInstPrinter.h"
 #include "MCTargetDesc/LoongArchMCExpr.h"
 #include "MCTargetDesc/LoongArchMCTargetDesc.h"
@@ -1560,9 +1561,10 @@ bool LoongArchAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
 
 unsigned LoongArchAsmParser::checkTargetMatchPredicate(MCInst &Inst) {
   unsigned Opc = Inst.getOpcode();
+  const MCInstrDesc &MCID = MII.get(Opc);
   switch (Opc) {
   default:
-    if (Opc >= LoongArch::AMADD_D && Opc <= LoongArch::AMXOR_W) {
+    if (LoongArchII::isSubjectToAMORdConstraint(MCID.TSFlags)) {
       MCRegister Rd = Inst.getOperand(0).getReg();
       MCRegister Rk = Inst.getOperand(1).getReg();
       MCRegister Rj = Inst.getOperand(2).getReg();
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td b/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
index 6ffc8823baee01..f49638c0ca93e7 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
@@ -32,6 +32,11 @@ class LAInst<dag outs, dag ins, string opcstr, string opnstr,
   let InOperandList = ins;
   let AsmString = opcstr # "\t" # opnstr;
   let Pattern = pattern;
+
+  // Target-specific instruction info and defaults
+
+  bit IsSubjectToAMORdConstraint = 0;
+  let TSFlags{0} = IsSubjectToAMORdConstraint;
 }
 
 // Pseudo instructions
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
index 671b8cc6ffe1b1..c7d2f0c8efb26f 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
@@ -713,7 +713,9 @@ class STORE_2RI14<bits<32> op>
 let hasSideEffects = 0, mayLoad = 1, mayStore = 1, Constraints = "@earlyclobber $rd" in
 class AM_3R<bits<32> op>
     : Fmt3R<op, (outs GPR:$rd), (ins GPR:$rk, GPRMemAtomic:$rj),
-            "$rd, $rk, $rj">;
+            "$rd, $rk, $rj"> {
+  let IsSubjectToAMORdConstraint = 1;
+}
 
 let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
 class LLBase<bits<32> op>
diff --git a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
index 1a12fb492a60f5..e06138a31bc022 100644
--- a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
+++ b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
@@ -56,6 +56,29 @@ enum {
   MO_DESC_CALL,
   // TODO: Add more flags.
 };
+
+// Target-specific flags of LAInst.
+// All definitions must match LoongArchInstrFormats.td.
+enum {
+  // Whether the instruction's rd is normally required to differ from rj and
+  // rk, in the way the 3-register atomic memory operations behave
+  // (Section 2.2.7.1 and 2.2.7.2, LoongArch Reference Manual Volume 1 v1.10;
+  // while Section 2.2.7.3 lacked similar description for the AMCAS
+  // instructions, at least the INE exception is still signaled on Loongson
+  // 3A6000 when its rd == rj).
+  //
+  // Used for generating diagnostics for assembler input that violate the
+  // constraint. As described on the manual, the covered instructions require
+  // rd != rj && rd != rk to work as intended.
+  IsSubjectToAMORdConstraintShift = 0,
+  IsSubjectToAMORdConstraintMask = 1 << IsSubjectToAMORdConstraintShift,
+};
+
+/// \returns true if this instruction's rd is normally required to differ
+/// from rj and rk, in the way 3-register atomic memory operations behave.
+static inline bool isSubjectToAMORdConstraint(uint64_t TSFlags) {
+  return TSFlags & IsSubjectToAMORdConstraintMask;
+}
 } // end namespace LoongArchII
 
 namespace LoongArchABI {
diff --git a/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s b/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
index 1c1c658ad440f8..0d81ddd6763b56 100644
--- a/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
+++ b/llvm/test/MC/LoongArch/Basic/Integer/invalid64.s
@@ -91,3 +91,14 @@ amxor.w $a0, $a1, $a0
 amadd.d $a0, $a1, $a2, $a3
 # CHECK: :[[#@LINE+1]]:24: error: optional integer offset must be 0
 amadd.d $a0, $a1, $a2, 1
+
+## According to experiment results on real LA664 HW, the AMCAS instructions
+## are subject to the same constraint as the other 3-register atomic insns.
+## This is undocumented in v1.10 of the LoongArch Reference Manual.
+
+# CHECK: :[[#@LINE+1]]:10: error: $rd must be different from both $rk and $rj
+amcas.b $a0, $a0, $a0
+# CHECK: :[[#@LINE+1]]:10: error: $rd must be different from both $rk and $rj
+amcas.h $a0, $a0, $a1
+# CHECK: :[[#@LINE+1]]:13: error: $rd must be different from both $rk and $rj
+amcas_db.w $a0, $a1, $a0

Footnotes

  1. if rd == rj an INE would be signaled; if rd == rk it is UB.

@xen0n
Copy link
Contributor Author

xen0n commented Oct 31, 2024

cc @SixWeining @wangleiat @heiher @xry111

(This was noticed during review of #114189.)

@xry111
Copy link
Contributor

xry111 commented Oct 31, 2024

The LoongArch Reference Manual says that the 3-register atomic memory operations cannot have their rd equal to either rj or rk 1, and both GNU as and LLVM IAS enforce the constraint.

This is not true. gas 2.43.50.20241031 still accepts amswap.w $r0, $r0, $r0.

And I prefer to provide the users a way to raise an INE.

@xen0n
Copy link
Contributor Author

xen0n commented Oct 31, 2024

The LoongArch Reference Manual says that the 3-register atomic memory operations cannot have their rd equal to either rj or rk 1, and both GNU as and LLVM IAS enforce the constraint.

This is not true. gas 2.43.50.20241031 still accepts amswap.w $r0, $r0, $r0.

And I prefer to provide the users a way to raise an INE.

Ah of course I meant (rd == rj || rd == rk) && (rd != 0) for the error condition.

I'd also appreciate an explicit INE trigger for power users. But it would probably require some spec work to avoid providing the footgun too easily. For now though they can always .word 0x38600400 for that amswap.w $r0, $r1, $r0... EDIT: I've misunderstood the text and right now amXXX $r0, XX, $r0 is the special-cased INE trigger form, nothing needs change.

@xen0n xen0n force-pushed the loong-am-djk-constraint branch from b96945e to af8b90a Compare October 31, 2024 13:35
@xry111
Copy link
Contributor

xry111 commented Oct 31, 2024

Ah, OK then...


let hasSideEffects = 0, mayLoad = 1, mayStore = 1, Constraints = "@earlyclobber $rd" in
class AM_3R<bits<32> op>
: Fmt3R<op, (outs GPR:$rd), (ins GPR:$rk, GPRMemAtomic:$rj),
Copy link
Member

Choose a reason for hiding this comment

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

AM_3R is incompatible with amcas,
Unlike other am** instructions, amcas rd rk rj , rd should be read, pervious version consider rd write only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AM_3R is incompatible with amcas, Unlike other am** instructions, amcas rd rk rj , rd should be read, pervious version consider rd write only.

Thanks for the suggestion, I have posted #114508 to precede this PR.

xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 1, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. No
functional change.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
@xen0n xen0n force-pushed the loong-am-djk-constraint branch from af8b90a to 0f60473 Compare November 1, 2024 06:31
@github-actions
Copy link

github-actions bot commented Nov 1, 2024

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

@xen0n xen0n force-pushed the loong-am-djk-constraint branch from 0f60473 to b77dc79 Compare November 1, 2024 06:34
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 1, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. No
functional change.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
@xen0n xen0n force-pushed the loong-am-djk-constraint branch from b77dc79 to 4f1644e Compare November 1, 2024 06:56
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 1, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 2, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
@xen0n xen0n force-pushed the loong-am-djk-constraint branch from 4f1644e to a311196 Compare November 2, 2024 02:33
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 2, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
@SixWeining SixWeining requested a review from heiher November 6, 2024 06:14
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 10, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
xen0n added a commit to xen0n/llvm-project that referenced this pull request Nov 18, 2024
The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link: llvm#114398 (comment)
@xen0n xen0n force-pushed the loong-am-djk-constraint branch from a311196 to 5422ed0 Compare November 18, 2024 06:11
Copy link
Member

@heiher heiher left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

SixWeining pushed a commit that referenced this pull request Nov 19, 2024
…14508)

The `rd` operand of AMCAS instructions is both read and written, because
of the nature of compare-and-swap operations, but currently it is not
declared as such. Fix it for upcoming codegen enablement changes. In
order to do that, a piece of LoongArchAsmParser logic that relied on
TableGen-erated enum variants being ordered in a specific way needs
updating; this will be addressed in a following refactor. No functional
change intended.

While at it, restore vertical alignment for the definition lines.

Suggested-by: tangaac <[email protected]>
Link:
#114398 (comment)
The LoongArch Reference Manual says that the 3-register atomic memory
operations cannot have their rd equal to either rj or rk [^1], and
both GNU as and LLVM IAS enforce the constraint for non-zero rd.
However, currently LoongArch AsmParser is checking for the opcode with
a direct numerical comparison on the opcode, which is enum-typed: the
fact that all AMO insns have adjacent numerical values is merely a
coincidence, and it is better to not rely on the current TableGen
implementation behavior.

Instead, start to leverage the target-specific flags field of
MCInstrDesc, and record the constraint with TableGen, so we can stop
treating the opcode value as number. In doing so, we also have to mark
whether the instruction is AMCAS, because the operand index of rj and
rk for the AMCAS instructions is different.

While documenting the new flag, it was found that v1.10 of the Manual
did not specify the similar constraint for the AMCAS instructions.
Experiments were done on a Loongson 3A6000 (LA664 uarch) and it turned
out that at least AMCAS will still signal INE with `rd == rj`. The
`rd == rk` case should be a no-op according to the semantics, but as it
is meaningless to perform CAS with the "old value" same as the "new
value", it is not worth special-casing. So the current behavior of also
enforcing the constraint for AMCAS is kept.

[^1]: if `rd == rj` an INE would be signaled; if `rd == rk` it is UB.
@xen0n xen0n force-pushed the loong-am-djk-constraint branch from 5422ed0 to 0e8bc8f Compare November 19, 2024 13:44
Copy link
Contributor

@SixWeining SixWeining left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks. The CI failure should be irrelevant.

@SixWeining SixWeining merged commit 8234c61 into llvm:main Nov 20, 2024
6 of 8 checks passed
@xen0n xen0n deleted the loong-am-djk-constraint branch November 20, 2024 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:loongarch llvm:mc Machine (object) code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants