Allow MAY(R)? to accept the high components of register pairs #98606

dominik-steenken · 2024-07-12T09:24:17Z

The HFP instructions MAY and MAYR, unlike any other floating point instructions, allow the specification of a 128bit register pair by either the lower-numbered or the higher-numbered component register. In order to support this, but change as little about codegen as possible, the existing MAY(R)? definition is made CodeGenOnly, while a copy is provided for the assembler and disassembler, which simply accepts a 64bit floating point register in place of the 128bit one. This copy is stripped of its pattern to prevent codegen from using it.
The corresponding assembly tests that checked the register specification rule that this commit removes from MAY(R)? have also been removed.

dominik-steenken · 2024-07-12T09:24:48Z

@uweigand @JonPsson1 FYI

llvmbot · 2024-07-12T09:24:50Z

@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-mc

Author: Dominik Steenken (dominik-steenken)

Changes

The HFP instructions MAY and MAYR, unlike any other floating point instructions, allow the specification of a 128bit register pair by either the lower-numbered or the higher-numbered component register. In order to support this, but change as little about codegen as possible, the existing MAY(R)? definition is made CodeGenOnly, while a copy is provided for the assembler and disassembler, which simply accepts a 64bit floating point register in place of the 128bit one. This copy is stripped of its pattern to prevent codegen from using it.
The corresponding assembly tests that checked the register specification rule that this commit removes from MAY(R)? have also been removed.

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

2 Files Affected:

(modified) llvm/lib/Target/SystemZ/SystemZInstrHFP.td (+15-2)
(modified) llvm/test/MC/SystemZ/insn-bad.s (-8)

diff --git a/llvm/lib/Target/SystemZ/SystemZInstrHFP.td b/llvm/lib/Target/SystemZ/SystemZInstrHFP.td
index d2e05b63c6c63..965dfddb7ef0f 100644
--- a/llvm/lib/Target/SystemZ/SystemZInstrHFP.td
+++ b/llvm/lib/Target/SystemZ/SystemZInstrHFP.td
@@ -209,13 +209,26 @@ def MYH  : BinaryRXF<"myh",  0xED3D, null_frag, FP64,  FP64, z_load, 8>;
 def MYL  : BinaryRXF<"myl",  0xED39, null_frag, FP64,  FP64, z_load, 8>;
 
 // Fused multiply-add (unnormalized).
-def MAYR  : TernaryRRD<"mayr",  0xB33A, null_frag, FP128, FP64>;
 def MAYHR : TernaryRRD<"mayhr", 0xB33C, null_frag, FP64,  FP64>;
 def MAYLR : TernaryRRD<"maylr", 0xB338, null_frag, FP64,  FP64>;
-def MAY   : TernaryRXF<"may",   0xED3A, null_frag, FP128, FP64, z_load, 8>;
 def MAYH  : TernaryRXF<"mayh",  0xED3C, null_frag, FP64,  FP64, z_load, 8>;
 def MAYL  : TernaryRXF<"mayl",  0xED38, null_frag, FP64,  FP64, z_load, 8>;
 
+// MAY and MAYR allow the user to specify the floating point register pair making 
+// up the FP128 register by either the lower-numbered register or the higher-numbered
+// register, in contrast to all other floating point instructions. For this
+// reason, the codegen and assembly versions of this instruction are kept separate
+// in order to allow the assembler and disassembler to accept these registers
+// without having to fundamentally change the instruction itself.
+let isCodeGenOnly = 1 in {
+  def MAY   : TernaryRXF<"may",   0xED3A, null_frag, FP128, FP64, z_load, 8>;
+  def MAYR  : TernaryRRD<"mayr",  0xB33A, null_frag, FP128, FP64>;
+}
+let hasNoSchedulingInfo = 1, OpKey = "", OpType = "", Pattern = [] <dag> in {
+  def MAY_Asm   : TernaryRXF<"may",   0xED3A, null_frag, FP64, FP64, z_load, 8>;
+  def MAYR_Asm  : TernaryRRD<"mayr",  0xB33A, null_frag, FP64, FP64>;
+}
+
 // Division.
 def DER : BinaryRR <"der", 0x3D,   null_frag, FP32,  FP32>;
 def DDR : BinaryRR <"ddr", 0x2D,   null_frag, FP64,  FP64>;
diff --git a/llvm/test/MC/SystemZ/insn-bad.s b/llvm/test/MC/SystemZ/insn-bad.s
index 6f94731fa0871..f81278610c73a 100644
--- a/llvm/test/MC/SystemZ/insn-bad.s
+++ b/llvm/test/MC/SystemZ/insn-bad.s
@@ -4176,12 +4176,9 @@
 #CHECK: may	%f0, %f0, -1
 #CHECK: error: invalid operand
 #CHECK: may	%f0, %f0, 4096
-#CHECK: error: invalid register pair
-#CHECK: may	%f2, %f0, 0
 
 	may	%f0, %f0, -1
 	may	%f0, %f0, 4096
-	may	%f2, %f0, 0
 
 #CHECK: error: invalid operand
 #CHECK: mayh	%f0, %f0, -1
@@ -4199,11 +4196,6 @@
 	mayl	%f0, %f0, -1
 	mayl	%f0, %f0, 4096
 
-#CHECK: error: invalid register pair
-#CHECK: mayr	%f2, %f0, %f0
-
-	mayr	%f2, %f0, %f0
-
 #CHECK: error: invalid operand
 #CHECK: mc	-1, 0
 #CHECK: error: invalid operand

uweigand · 2024-07-12T11:13:56Z

llvm/lib/Target/SystemZ/SystemZInstrHFP.td

+  def MAY   : TernaryRXF<"may",   0xED3A, null_frag, FP128, FP64, z_load, 8>;
+  def MAYR  : TernaryRRD<"mayr",  0xB33A, null_frag, FP128, FP64>;
+}
+let hasNoSchedulingInfo = 1, OpKey = "", OpType = "", Pattern = [] <dag> in {


You don't need to reset Pattern here - since null_frag is passed, the pattern in TernaryRXY etc can never match anything anyway. B.t.w. because of that, these patterns can currently never be used for code-gen, so the isCodeGenOnly variant you're adding can actually never be used for anything - I think it would be preferable to leave it out.

Hm, ok so the preferred fix here would be to just replace the FP128 with a FP64 in the existing MAY/MAYR definition? We would have to remember that this is technically the wrong register class if we ever want to enable code generation for these, right?

Hm, ok so the preferred fix here would be to just replace the FP128 with a FP64 in the existing MAY/MAYR definition?

Yes.

We would have to remember that this is technically the wrong register class if we ever want to enable code generation for these, right?

Please add a comment pointing that out.

uweigand · 2024-07-12T13:44:01Z

llvm/test/MC/SystemZ/insn-bad.s


 	may	%f0, %f0, -1
 	may	%f0, %f0, 4096
-	may	%f2, %f0, 0


Oh, one more thing: in addition to removing the insn-bad error check, can you please add a positive test to insn-good that this is actually accepted? This is quite unusual, and it would be good to have a test that catches any accidental regression here.

The HFP instructions MAY and MAYR, unlike any other floating point instruction, allow the specification of a 128bit register pair by either the lower-numbered or the higher-numbered component register. In order to support this, the existing MAY(R)? definition is changed to simply accept a 64bit floating point register in place of the 128bit one. Since the instruction is currently not used for code generation, this is safe. A comment is added to ensure that this is taken into account if and when these instructions are enabled for CodeGen. The corresponding assembly tests that checked the register specification rule that this commit removes from MAY(R)? are also deleted, and conversely, tests to check for the acceptance of the previously forbidden encodings are added.

uweigand

LGTM, thanks!

Summary: The HFP instructions `MAY` and `MAYR`, unlike any other floating point instructions, allow the specification of a 128bit register pair by either the lower-numbered or the higher-numbered component register. In order to support this, but change as little about codegen as possible, the existing `MAY(R)?` definition is made `CodeGenOnly`, while a copy is provided for the assembler and disassembler, which simply accepts a 64bit floating point register in place of the 128bit one. This copy is stripped of its pattern to prevent codegen from using it. The corresponding assembly tests that checked the register specification rule that this commit removes from `MAY(R)?` have also been removed. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250787

llvmbot added backend:SystemZ mc Machine (object) code labels Jul 12, 2024

uweigand reviewed Jul 12, 2024

View reviewed changes

dominik-steenken force-pushed the fix-may-mayr branch from a2c934a to f46084c Compare July 18, 2024 09:00

uweigand approved these changes Jul 18, 2024

View reviewed changes

uweigand merged commit 3d69bbc into llvm:main Jul 18, 2024
7 checks passed

dominik-steenken deleted the fix-may-mayr branch March 5, 2025 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow MAY(R)? to accept the high components of register pairs #98606

Allow MAY(R)? to accept the high components of register pairs #98606

Uh oh!

dominik-steenken commented Jul 12, 2024

Uh oh!

dominik-steenken commented Jul 12, 2024

Uh oh!

llvmbot commented Jul 12, 2024 •

edited

Loading

Uh oh!

uweigand Jul 12, 2024

Uh oh!

dominik-steenken Jul 12, 2024

Uh oh!

uweigand Jul 12, 2024

Uh oh!

dominik-steenken Jul 18, 2024

Uh oh!

uweigand Jul 12, 2024

Uh oh!

dominik-steenken Jul 18, 2024

Uh oh!

uweigand left a comment

Uh oh!

Uh oh!

Uh oh!

Allow MAY(R)? to accept the high components of register pairs #98606

Allow MAY(R)? to accept the high components of register pairs #98606

Uh oh!

Conversation

dominik-steenken commented Jul 12, 2024

Uh oh!

dominik-steenken commented Jul 12, 2024

Uh oh!

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

Uh oh!

Uh oh!

uweigand Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

dominik-steenken Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

uweigand Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

dominik-steenken Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

uweigand Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

dominik-steenken Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

uweigand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 12, 2024 •

edited

Loading