[SystemZ] Accept plain register name where an address is expected.

[jonpa]

Summary

Allow an address to be written as just a single (base) register in assembly.

Example: 'lg %r0, 0(%r1)' and 'lg %r0, %r1' are equivalent.

This patch is not checking the expected kind of address (MemKind) or HasLength / HasVectorIndex - would that be better?

[uweigand]

This patch is not checking the expected kind of address (MemKind) or HasLength / HasVectorIndex - would that be better?

I believe %reg should be treated equivalently to 0(%reg) for any type of address, including those with length or vector index, so this should be fine. However, it would be good to validate that behavior against GAS, and also add a few test cases for instructions with those address types.

Otherwise the patch looks good to me.

[jonpa]

I believe %reg should be treated equivalently to 0(%reg) for any type of address, including those with length or vector index, so this should be fine. However, it would be good to validate that behavior against GAS, and also add a few test cases for instructions with those address types.

For addresses with a Length field, it seems GAS does require the full address with the explicit displacement as well:

        xc      0(32,%r2), 0(%r2)
        xc      0(32,%r2), %r2
#       xc      (32,%r2), %r2    # not accepted by GAS
#       xc      32,%r2, %r2      # not accepted by GAS

The same is also the case with a vector index:

         vsceg   %v0, 0(%v1, %r3), 1
#        vsceg   %v0, (%v1, %r3), 1  # not accepted by GAS
#        vsceg   %v0, %v1, %r3, 1    # not accepted by GAS

So I did not see any changes I could make for the patch, but I nevertheless added an extra test for the second (plain) address of an XC.

An interesting twist is that you have to use the full name (%rX); if you were to use just the register number (1-15), it would be considered a displacement instead of a register ...

Hmmm... to my surprise it now seems that GAS is treating a plain register name as an offset:

lg      %r0, 0(%r1)
lg      %r0, %r1
lg      %r0, 1

-> as ; objdump ->

Disassembly of section .text:

0000000000000000 <.text>:
   0:	e3 00 10 00 00 04 	lg	%r0,0(%r1)
   6:	e3 00 00 01 00 04 	lg	%r0,1
   c:	e3 00 00 01 00 04 	lg	%r0,1

With this patch though as we agreed:

llvm-mc --show-encoding
	.text
	lg	%r0, 0(%r1)                     # encoding: [0xe3,0x00,0x10,0x00,0x00,0x04]
	lg	%r0, 0(%r1)                     # encoding: [0xe3,0x00,0x10,0x00,0x00,0x04]
	lg	%r0, 1                          # encoding: [0xe3,0x00,0x00,0x01,0x00,0x04]

Mayebe this was fixed recently in GAS?

as --version
GNU assembler version 2.32-30.2ibm.fc31
Copyright (C) 2019 Free Software Foundation, Inc.

[uweigand]

In D109657#3008172, @jonpa wrote:

I believe %reg should be treated equivalently to 0(%reg) for any type of address, including those with length or vector index, so this should be fine. However, it would be good to validate that behavior against GAS, and also add a few test cases for instructions with those address types.

For addresses with a Length field, it seems GAS does require the full address with the explicit displacement as well:

        xc      0(32,%r2), 0(%r2)
        xc      0(32,%r2), %r2
#       xc      (32,%r2), %r2    # not accepted by GAS
#       xc      32,%r2, %r2      # not accepted by GAS

Yes, if you have a length field, you need the full address. I was wondering whether you can omit the length field completely, e.g. have something like

xc %r2, %r2

but it looks like this is isn't accepted anyway as the length is mandatory (makes sense to me as well).

An interesting twist is that you have to use the full name (%rX); if you were to use just the register number (1-15), it would be considered a displacement instead of a register ...

Hmmm... to my surprise it now seems that GAS is treating a plain register name as an offset:

lg      %r0, 0(%r1)
lg      %r0, %r1
lg      %r0, 1

-> as ; objdump ->

Disassembly of section .text:

0000000000000000 <.text>:
   0:	e3 00 10 00 00 04 	lg	%r0,0(%r1)
   6:	e3 00 00 01 00 04 	lg	%r0,1
   c:	e3 00 00 01 00 04 	lg	%r0,1

With this patch though as we agreed:

llvm-mc --show-encoding
	.text
	lg	%r0, 0(%r1)                     # encoding: [0xe3,0x00,0x10,0x00,0x00,0x04]
	lg	%r0, 0(%r1)                     # encoding: [0xe3,0x00,0x10,0x00,0x00,0x04]
	lg	%r0, 1                          # encoding: [0xe3,0x00,0x00,0x01,0x00,0x04]

Huh, this is a problem. We'll need to verify this with the binutils team -- that behavior seems just broken to me. (And if the current behavior of GAS is just broken, then I'm wondering whether the correct fix isn't rather to make this an error in GAS too, and not change LLVM at all ...)

llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp

bool SystemZAsmParser::parseIntegerRegister(Register &Reg,
   return false;
 }
 
 // Parse a memory operand into Reg1, Reg2, Disp, and Length.
 bool SystemZAsmParser::parseAddress(bool &HaveReg1, Register &Reg1,
                                     bool &HaveReg2, Register &Reg2,
                                     const MCExpr *&Disp, const MCExpr *&Length,
                                     bool HasLength, bool HasVectorIndex) {
-  // Parse the displacement, which must always be present.
-  if (getParser().parseExpression(Disp))
-    return true;
-
-  // Parse the optional base and index.
   HaveReg1 = false;
   HaveReg2 = false;
   Length = nullptr;
 
+  // Parse a single register as a complete address.
+  if (getLexer().is(AsmToken::Percent)) {
+    if (parseRegister(Reg1))
+      return true;
+    HaveReg1 = true;
+    Disp = MCConstantExpr::create(0, getContext());
+    return false;
+  }
+
+  // Parse the displacement, which at this point must always be present.
+  if (getParser().parseExpression(Disp))
+    return true;
+
+  // Parse the optional base and index.
+  //
   // If we have a scenario as below:
   //   vgef %v0, 0(0), 0
   // This is an example of a "BDVMem" instruction type.
   //
   // So when we parse this as an integer register, the register group
   // needs to be tied to "RegV". Usually when the prefix is passed in
   // as %<prefix><reg-number> its easy to check which group it should belong to
   // However, if we're passing in just the integer there's no real way to

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

 # For z10 and above.
 # RUN: llvm-mc -triple s390x-linux-gnu -show-encoding %s | FileCheck %s
 
 #CHECK: a %r0, 0                  # encoding: [0x5a,0x00,0x00,0x00]
 #CHECK: a %r0, 4095               # encoding: [0x5a,0x00,0x0f,0xff]
 #CHECK: a %r0, 0(%r1)             # encoding: [0x5a,0x00,0x10,0x00]
 #CHECK: a %r0, 0(%r15)            # encoding: [0x5a,0x00,0xf0,0x00]
+#CHECK: a %r0, 0(%r15)            # encoding: [0x5a,0x00,0xf0,0x00]
 #CHECK: a %r0, 4095(%r1,%r15)     # encoding: [0x5a,0x01,0xff,0xff]
 #CHECK: a %r0, 4095(%r15,%r1)     # encoding: [0x5a,0x0f,0x1f,0xff]
 #CHECK: a %r15, 0                 # encoding: [0x5a,0xf0,0x00,0x00]
 #CHECK: a %r0, 0                  # encoding: [0x5a,0x00,0x00,0x00]
 #CHECK: a %r0, 4095               # encoding: [0x5a,0x00,0x0f,0xff]
 #CHECK: a %r0, 0(%r1)             # encoding: [0x5a,0x00,0x10,0x00]
 #CHECK: a %r0, 0(%r15)            # encoding: [0x5a,0x00,0xf0,0x00]
 #CHECK: a %r0, 4095(%r1,%r15)     # encoding: [0x5a,0x01,0xff,0xff]
 #CHECK: a %r0, 4095(%r15,%r1)     # encoding: [0x5a,0x0f,0x1f,0xff]
 #CHECK: a %r15, 0                 # encoding: [0x5a,0xf0,0x00,0x00]
 
   a %r0, 0
   a %r0, 4095
   a %r0, 0(%r1)
   a %r0, 0(%r15)
+  a %r0, %r15
   a %r0, 4095(%r1,%r15)
   a %r0, 4095(%r15,%r1)
   a %r15, 0
   a 0, 0
   a 0, 4095
   a 0, 0(1)
   a 0, 0(15)
   a 0, 4095(1,15)
#CHECK: ad  %f15, 0                 # encoding: [0x6a,0xf0,0x00,0x00]
   ad  %f0, 4095(%r1,%r15)
   ad  %f0, 4095(%r15,%r1)
   ad  %f15, 0
 
 #CHECK: adb %f0, 0                  # encoding: [0xed,0x00,0x00,0x00,0x00,0x1a]
 #CHECK: adb %f0, 4095               # encoding: [0xed,0x00,0x0f,0xff,0x00,0x1a]
 #CHECK: adb %f0, 0(%r1)             # encoding: [0xed,0x00,0x10,0x00,0x00,0x1a]
 #CHECK: adb %f0, 0(%r15)            # encoding: [0xed,0x00,0xf0,0x00,0x00,0x1a]
+#CHECK: adb %f0, 0(%r15)            # encoding: [0xed,0x00,0xf0,0x00,0x00,0x1a]
 #CHECK: adb %f0, 4095(%r1,%r15)     # encoding: [0xed,0x01,0xff,0xff,0x00,0x1a]
 #CHECK: adb %f0, 4095(%r15,%r1)     # encoding: [0xed,0x0f,0x1f,0xff,0x00,0x1a]
 #CHECK: adb %f15, 0                 # encoding: [0xed,0xf0,0x00,0x00,0x00,0x1a]
 
   adb %f0, 0
   adb %f0, 4095
   adb %f0, 0(%r1)
   adb %f0, 0(%r15)
+  adb %f0, %r15
   adb %f0, 4095(%r1,%r15)
   adb %f0, 4095(%r15,%r1)
   adb %f15, 0
 
 #CHECK: adbr  %f0, %f0                # encoding: [0xb3,0x1a,0x00,0x00]
 #CHECK: adbr  %f0, %f15               # encoding: [0xb3,0x1a,0x00,0x0f]
 #CHECK: adbr  %f7, %f8                # encoding: [0xb3,0x1a,0x00,0x78]
 #CHECK: adbr  %f15, %f0               # encoding: [0xb3,0x1a,0x00,0xf0]

 
 #CHECK: lg  %r0, -524288            # encoding: [0xe3,0x00,0x00,0x00,0x80,0x04]
 #CHECK: lg  %r0, -1                 # encoding: [0xe3,0x00,0x0f,0xff,0xff,0x04]
 #CHECK: lg  %r0, 0                  # encoding: [0xe3,0x00,0x00,0x00,0x00,0x04]
 #CHECK: lg  %r0, 1                  # encoding: [0xe3,0x00,0x00,0x01,0x00,0x04]
 #CHECK: lg  %r0, 524287             # encoding: [0xe3,0x00,0x0f,0xff,0x7f,0x04]
 #CHECK: lg  %r0, 0(%r1)             # encoding: [0xe3,0x00,0x10,0x00,0x00,0x04]
 #CHECK: lg  %r0, 0(%r15)            # encoding: [0xe3,0x00,0xf0,0x00,0x00,0x04]
+#CHECK: lg  %r0, 0(%r15)            # encoding: [0xe3,0x00,0xf0,0x00,0x00,0x04]
 #CHECK: lg  %r0, 524287(%r1,%r15)   # encoding: [0xe3,0x01,0xff,0xff,0x7f,0x04]
 #CHECK: lg  %r0, 524287(%r15,%r1)   # encoding: [0xe3,0x0f,0x1f,0xff,0x7f,0x04]
 #CHECK: lg  %r15, 0                 # encoding: [0xe3,0xf0,0x00,0x00,0x00,0x04]
 
   lg  %r0, -524288
   lg  %r0, -1
   lg  %r0, 0
   lg  %r0, 1
   lg  %r0, 524287
   lg  %r0, 0(%r1)
   lg  %r0, 0(%r15)
+  lg  %r0, %r15
   lg  %r0, 524287(%r1,%r15)
   lg  %r0, 524287(%r15,%r1)
   lg  %r15, 0
 
 #CHECK: lgb %r0, -524288            # encoding: [0xe3,0x00,0x00,0x00,0x80,0x77]
 #CHECK: lgb %r0, -1                 # encoding: [0xe3,0x00,0x0f,0xff,0xff,0x77]
 #CHECK: lgb %r0, 0                  # encoding: [0xe3,0x00,0x00,0x00,0x00,0x77]
 #CHECK: lgb %r0, 1                  # encoding: [0xe3,0x00,0x00,0x01,0x00,0x77]
#CHECK: x %r15, 0                 # encoding: [0x57,0xf0,0x00,0x00]
   x %r0, 0(%r15)
   x %r0, 4095(%r1,%r15)
   x %r0, 4095(%r15,%r1)
   x %r15, 0
 
 #CHECK: xc  0(1), 0                 # encoding: [0xd7,0x00,0x00,0x00,0x00,0x00]
 #CHECK: xc  0(1), 0(%r1)            # encoding: [0xd7,0x00,0x00,0x00,0x10,0x00]
 #CHECK: xc  0(1), 0(%r15)           # encoding: [0xd7,0x00,0x00,0x00,0xf0,0x00]
+#CHECK: xc  0(1), 0(%r15)           # encoding: [0xd7,0x00,0x00,0x00,0xf0,0x00]
 #CHECK: xc  0(1), 4095              # encoding: [0xd7,0x00,0x00,0x00,0x0f,0xff]
 #CHECK: xc  0(1), 4095(%r1)         # encoding: [0xd7,0x00,0x00,0x00,0x1f,0xff]
 #CHECK: xc  0(1), 4095(%r15)        # encoding: [0xd7,0x00,0x00,0x00,0xff,0xff]
 #CHECK: xc  0(1,%r1), 0             # encoding: [0xd7,0x00,0x10,0x00,0x00,0x00]
 #CHECK: xc  0(1,%r15), 0            # encoding: [0xd7,0x00,0xf0,0x00,0x00,0x00]
 #CHECK: xc  4095(1,%r1), 0          # encoding: [0xd7,0x00,0x1f,0xff,0x00,0x00]
 #CHECK: xc  4095(1,%r15), 0         # encoding: [0xd7,0x00,0xff,0xff,0x00,0x00]
 #CHECK: xc  0(256,%r1), 0           # encoding: [0xd7,0xff,0x10,0x00,0x00,0x00]
 #CHECK: xc  0(256,%r15), 0          # encoding: [0xd7,0xff,0xf0,0x00,0x00,0x00]
 
   xc  0(1), 0
   xc  0(1), 0(%r1)
   xc  0(1), 0(%r15)
+  xc  0(1), %r15
   xc  0(1), 4095
   xc  0(1), 4095(%r1)
   xc  0(1), 4095(%r15)
   xc  0(1,%r1), 0
   xc  0(1,%r15), 0
   xc  4095(1,%r1), 0
   xc  4095(1,%r15), 0
   xc  0(256,%r1), 0