[AArch64] Support expression results as immediate values in mov

[jcai19]

Summary

This patch adds support of using the result of an expression as an
immediate value. For example,

0:
.skip 4
1:
mov x0, 1b - 0b

is assembled to

mov x0, #4

Currently it does not support expressions requiring relocation unless
explicitly specified. This fixes PR#45781.

[jcai19]

Support the cases when the result of subtraction between two labels are negative.

[ostannard]

• Testing that the expression is the subtraction of two SymbolRefs seems needlessly restrictive, could we accept any expression, and reject it in the AsmBackend if it can't be fully resolved or converted to a relocation?
• Does this check that the value fits into a 16 bit immediate? The GNU assembler also seems to accept expressions which fit into a MOVZ/MOVN with a shift, though I can't see that being very useful for expressions involving symbol addresses.
• This needs tests.

[jcai19]

Remove the restraction of supporting subtraction of two symbols only.

[jcai19]

Thank you for the comments! Please find my replies inlined.

In D80028#2044356, @ostannard wrote:

• Testing that the expression is the subtraction of two SymbolRefs seems needlessly restrictive, could we accept any expression, and reject it in the AsmBackend if it can't be fully resolved or converted to a relocation?

Sounds good. I have relaxed the restriction, but the patch still can't seem to deal with cases like mov reg, label. Will take a look at that.

• Does this check that the value fits into a 16 bit immediate? The GNU assembler also seems to accept expressions which fit into a MOVZ/MOVN with a shift, though I can't see that being very useful for expressions involving symbol addresses.

I mistakenly thought the bound has been checked. Double checking it again though it seemed the check was missing. I have added it.

• This needs tests.

Added. Will add more cases once the patch supports more label-based expressions as an immediate operand.

[jcai19]

Add a comment.

[efriedma]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

What happens when Shift isn't zero?

1785

Why are we adding two operands here?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

250

I'd prefer to explicitly compute a RefKind here, as opposed to inferring that a null RefKind has some specific meaning.

[jcai19]

Update based on comments.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

Thank you for the comments. And sorry for the delay, I was trying to look into the cases where the value of Shift is not zero. However, even gcc could not support mov instructions with shift operand.

$ cat foo.s
mov x0, 0x1, lsl 16

$ aarch64-linux-gnu-gcc -c foo.s
foo.s: Assembler messages:
foo.s:1: Error: unexpected characters following instruction at operand 2 -- `mov x0,0x1,lsl 16'

So far the only cases I found with non-zero Shift is during instruction matching stage, when different values of Shift are tried out, and that should not affect this code. Are there additional cases that would cause Shift to not be zero?

[jcai19]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

250

We don't currently have a RefKind corresponding to zero. Should we add one?

[jcai19]

Fix a typo.

[efriedma]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

This codepath triggers for constructs like "mov x0, 0x10000".

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

250

I haven't looked carefully at the implications, but I think we should wrap the expressions in an AArch64MCExpr so we can set an appropriate RefKind.

[jcai19]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

Thanks for the example. Since we do not know the result of symbol arithmetic at parsing time so if the result is out of [-0xffff, 0xfff] range, this patch wouldn't be able to handle the cases where Shift is not 0. GNU assembler can handle those cases, and I assume that is because GAS use a multi-iteration design so it can check whether the value is valid after the expression is evaluated, unlike the one-iteration design used by the IAS. I am not sure if there will be many use cases requiring the support of a bigger range. @ostannard previously mentioned

The GNU assembler also seems to accept expressions which fit into a MOVZ/MOVN with a shift, though I can't see that being very useful for expressions involving symbol addresses.

Maybe this is okay?

[efriedma]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

It's probably okay.

Please make the code here actually verify that Shift is zero, though, instead of just assuming it's zero.

[jcai19]

Add the expression operand as AArch64MCExpr.

[efriedma]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

Please also add a testcase for something like mov x0, 0b, where the value can't be resolved in the assembler.

[jcai19]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

974

Thanks for the clarification.

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

This is actually interesting. GAS assembled the following code

0:
.skip 4
1:
mov x0, 1b

into

mov x0, #0x4

With the proposed change, IAS would calculate the offset (4) correctly but discard it as it considers the expression 1b unresolved, and generate the following code.

mov x0, #0x0

What's the expected outcome here? Is subtraction between two symbols the only valid expressions we should accept? Thanks.

[efriedma]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

Converting the address of a label to constant without a relocation is clearly a bug; we shouldn't try to reproduce that behavior.

I think I'd prefer to be conservative and forbid expressions that would require a relocation in the object file, at least for now. (The user can spell it out with movz/movk if the actually want a relocation.)

[jcai19]

This comment was removed by jcai19.

[jcai19]

Revert to the implementation before the last version.

[jcai19]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

It seems IAS currently replaces all the labels that it cannot fully resolve with 0, like the following example.

$ cat foo.s
movz x13, #:abs_g0_s:some_label

$ llvm-mc -triple=aarch64 -filetype=obj --debug-only=asm-matcher foo.s -o foo.o
$ aarch64-linux-gnu-objdump -d foo.o
foo.o: file format elf64-littleaarch64

Disassembly of section .text:

0000000000000000 <.text>:

0:	d280000d 	mov	x13, #0x0                   	// #0

Adding the proposed check seemed to break all such cases. Shall we address that in a separate change?

[efriedma]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

It's not zero, it's a relocation. objdump -dr output:

0000000000000000 <.text>:
   0:   d280000d        mov     x13, #0x0                       // #0
                        0: R_AARCH64_MOVW_SABS_G0       some_label

I'm suggesting that we forbid emitting relocations specifically for the alias "mov", and allow them if someone requests them with "movz" etc.

[jcai19]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

I see, thank you for the clarification. Double checking again the current patch also emits relocation for instructions like "mov x0, 1b" too. I tried to add the check at the beginning of AArch64AsmBackend::applyFixup as this was the first backend function IAS called after the expressions are evaluated, but it broke the movz example I mentioned in previous comments. Do you have any suggestions as to where we can add this check for mov alias specifically? Also why can't we emit relocations for mov if we allow movz to do so? I thought mov immediate was an alias of movz on 64-bit ARM. Thanks.

[efriedma]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

Do you have any suggestions as to where we can add this check for mov alias specifically?

Hmm... maybe we should actually go back to doing something more like what the earlier versions did, and not create an AArch64MCExpr.

Also why can't we emit relocations for mov if we allow movz to do so?

It's essentially a question of usability. I'm concerned about confusing users: someone tries to use mov to compute the address of a function, the assembler eats it, and they eventually get an obscure link error complaining the value doesn't fit into the relocation.

If the user explicitly writes :abs_g0_s:, we can assume they know what they're doing.

[jcai19]

Do not create AArch64MCExpr.

[jcai19]

llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s

9 ↗

(On Diff #266393)

Thank you for the all clarification! I have updated the patch to not create AAarch64MCExpr.

[jcai19]

Fix a typo.

[efriedma]

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

979

E can't be null here?

Maybe worth briefly explaining here what this allows.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

267

Put the comment inside the if?

270

Can you make the error message specify the range we're looking for?

464

Please put parentheses around the "&&".

[jcai19]

Updates based on comments.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

979

the function would have returned at its begining if E is null. Comment added.

[jcai19]

@efriedma Does the current version of the patch look like it is good to go? Thanks.

[efriedma]

LGTM

[jcai19]

In D80028#2081258, @efriedma wrote:

LGTM

Thank you!

[kristof.beyls]

llvm/test/MC/AArch64/mov-expr-as-immediate.s

1 ↗

(On Diff #269392)

It seems that this test line has a race condition, and is causing bots to fail non-deterministically.
For example:
http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/30108/steps/test-check-all/logs/stdio
http://lab.llvm.org:8011/builders/fuchsia-x86_64-linux/builds/6224/steps/check/logs/stdio

The error message I get on a local test run (which sometimes passes, sometimes fails) is per below.
I think this indicates that instead of using a pipe (|), there should be a semi-colon (or something similar if lit does not support semi-colons) between the run of llvm-mc and the run of llvm-objdump.
It seems that in some invocations, the pipe results in llvm-objdump getting run before llvm-mc is finished writing to temporary file %t.

llvm/test/MC/AArch64/mov-expression-as-immediate.s seems to have the same issue.

If the above seems correct, could you fix this?

kribey01@hw-a20-17:~/dev/llvm.org/build2$ ./bin/llvm-lit -sv ~/dev/llvm.org/llvm-project/llvm/test/MC/AArch64/mov-expression-as-immediate.s
FAIL: LLVM :: MC/AArch64/mov-expression-as-immediate.s (1 of 1)
******************** TEST 'LLVM :: MC/AArch64/mov-expression-as-immediate.s' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/kribey01/dev/llvm.org/build2/bin/llvm-mc -triple aarch64-none-linux-gnu /home/kribey01/dev/llvm.org/llvm-project/llvm/test/MC/AArch64/mov-expression-as-immediate.s -filetype=obj -o /home/kribey01/dev/llvm.org/build2/test/MC/AArch64/Output/mov-expression-as-immediate.s.tmp | /home/kribey01/dev/llvm.org/build2/bin/llvm-objdump -d /home/kribey01/dev/llvm.org/build2/test/MC/AArch64/Output/mov-expression-as-immediate.s.tmp | /home/kribey01/dev/llvm.org/build2/bin/FileCheck /home/kribey01/dev/llvm.org/llvm-project/llvm/test/MC/AArch64/mov-expression-as-immediate.s
--
Exit Code: 2

Command Output (stderr):
--
/home/kribey01/dev/llvm.org/build2/bin/llvm-objdump: error: '/home/kribey01/dev/llvm.org/build2/test/MC/AArch64/Output/mov-expression-as-immediate.s.tmp': The file was not recognized as a valid object file
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/kribey01/dev/llvm.org/build2/bin/FileCheck /home/kribey01/dev/llvm.org/llvm-project/llvm/test/MC/AArch64/mov-expression-as-immediate.s

--

********************
********************
Failed Tests (1):
  LLVM :: MC/AArch64/mov-expression-as-immediate.s

[kristof.beyls]

I ended up pushing a fix for the racy tests in 7e6f891df85

[jcai19]

In D80028#2082105, @kristof.beyls wrote:

I ended up pushing a fix for the racy tests in 7e6f891df85

Thank you Kristof.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

return isMovWSymbol(
          AArch64MCExpr::VK_TPREL_G0, AArch64MCExpr::VK_TPREL_G0_NC,
          AArch64MCExpr::VK_DTPREL_G0, AArch64MCExpr::VK_DTPREL_G0_NC});
   }
 
   template<int RegWidth, int Shift>
   bool isMOVZMovAlias() const {
     if (!isImm()) return false;
 
-    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
-    if (!CE) return false;
+    const MCExpr *E = dyn_cast<MCExpr>(getImm());
+    if (const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(E)) {
       uint64_t Value = CE->getValue();
 
       return AArch64_AM::isMOVZMovAlias(Value, Shift, RegWidth);
     }
+    return E;
+  }
 
   template<int RegWidth, int Shift>
   bool isMOVNMovAlias() const {
     if (!isImm()) return false;
 
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     if (!CE) return false;
     uint64_t Value = CE->getValue();
void addMemExtend8Operands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::createImm(IsSigned));
     Inst.addOperand(MCOperand::createImm(hasShiftExtendAmount()));
   }
 
   template<int Shift>
   void addMOVZMovAliasOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
 
-    const MCConstantExpr *CE = cast<MCConstantExpr>(getImm());
+    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
+    if (CE) {
       uint64_t Value = CE->getValue();
       Inst.addOperand(MCOperand::createImm((Value >> Shift) & 0xffff));
+    } else {
+      addExpr(Inst, getImm());
+      Inst.addOperand(MCOperand::createImm(0));
+    }
   }
 
   template<int Shift>
   void addMOVNMovAliasOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
 
     const MCConstantExpr *CE = cast<MCConstantExpr>(getImm());
     uint64_t Value = CE->getValue();

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
       Value &= 0xfff;
     // Unsigned 12-bit immediate which gets multiplied by 16
     if (Value >= 0x10000)
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     if (Value & 0xf)
       Ctx.reportError(Fixup.getLoc(), "fixup must be 16-byte aligned");
     return Value >> 4;
   case AArch64::fixup_aarch64_movw: {
+    if (!IsResolved) {
+      // FIXME: Figure out when this can actually happen, and verify our
+      // behavior.
+      Ctx.reportError(Fixup.getLoc(), "unresolved movw fixup not yet "
+                                      "implemented");
+      return Value;
+    }
     AArch64MCExpr::VariantKind RefKind =
         static_cast<AArch64MCExpr::VariantKind>(Target.getRefKind());
+    if (!RefKind) {
+      if (SignedValue > 0xFFFF || SignedValue < -0xFFFF)
+        Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+
+      // Invert the negative immediate because it will feed into a MOVN.
+      if (SignedValue < 0)
+        SignedValue = ~SignedValue;
+      Value = static_cast<uint64_t>(SignedValue);
+      return Value;
+    }
     if (AArch64MCExpr::getSymbolLoc(RefKind) != AArch64MCExpr::VK_ABS &&
         AArch64MCExpr::getSymbolLoc(RefKind) != AArch64MCExpr::VK_SABS) {
       // VK_GOTTPREL, VK_TPREL, VK_DTPREL are movw fixups, but they can't
       // ever be resolved in the assembler.
       Ctx.reportError(Fixup.getLoc(),
                       "relocation for a thread-local variable points to an "
                       "absolute symbol");
       return Value;
     }
 
-    if (!IsResolved) {
-      // FIXME: Figure out when this can actually happen, and verify our
-      // behavior.
-      Ctx.reportError(Fixup.getLoc(), "unresolved movw fixup not yet "
-                                      "implemented");
-      return Value;
-    }
 
     if (AArch64MCExpr::getSymbolLoc(RefKind) == AArch64MCExpr::VK_SABS) {
       switch (AArch64MCExpr::getAddressFrag(RefKind)) {
       case AArch64MCExpr::VK_G0:
         break;
       case AArch64MCExpr::VK_G1:
         SignedValue = SignedValue >> 16;
         break;
for (unsigned i = 0; i != NumBytes; ++i) {
       Data[Offset + Idx] |= uint8_t((Value >> (i * 8)) & 0xff);
     }
   }
 
   // FIXME: getFixupKindInfo() and getFixupKindNumBytes() could be fixed to
   // handle this more cleanly. This may affect the output of -show-mc-encoding.
   AArch64MCExpr::VariantKind RefKind =
     static_cast<AArch64MCExpr::VariantKind>(Target.getRefKind());
+  if (!RefKind) {
+    if (Fixup.getTargetKind() == AArch64::fixup_aarch64_movw) {
+      if (SignedValue < 0)
+        Data[Offset + 3] &= ~(1 << 6);
+      else
+        Data[Offset + 3] |= (1 << 6);
+    }
+  }
   if (AArch64MCExpr::getSymbolLoc(RefKind) == AArch64MCExpr::VK_SABS) {
     // If the immediate is negative, generate MOVN else MOVZ.
     // (Bit 30 = 0) ==> MOVN, (Bit 30 = 1) ==> MOVZ.
     if (SignedValue < 0)
       Data[Offset + 3] &= ~(1 << 6);
     else
       Data[Offset + 3] |= (1 << 6);
   }
 }
 
 bool AArch64AsmBackend::mayNeedRelaxation(const MCInst &Inst,

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp

unsigned AArch64MCCodeEmitter::fixMOVZ(const MCInst &MI, unsigned EncodedValue,
   // job to ensure that any bits possibly affected by this are 0. This means we
   // must zero out bit 30 (essentially emitting a MOVN).
   MCOperand UImm16MO = MI.getOperand(1);
 
   // Nothing to do if there's no fixup.
   if (UImm16MO.isImm())
     return EncodedValue;
 
-  const AArch64MCExpr *A64E = cast<AArch64MCExpr>(UImm16MO.getExpr());
+  const MCExpr *E = UImm16MO.getExpr();
+  if (const AArch64MCExpr *A64E = dyn_cast<AArch64MCExpr>(E)) {
     switch (A64E->getKind()) {
     case AArch64MCExpr::VK_DTPREL_G2:
     case AArch64MCExpr::VK_DTPREL_G1:
     case AArch64MCExpr::VK_DTPREL_G0:
     case AArch64MCExpr::VK_GOTTPREL_G1:
     case AArch64MCExpr::VK_TPREL_G2:
     case AArch64MCExpr::VK_TPREL_G1:
     case AArch64MCExpr::VK_TPREL_G0:
       return EncodedValue & ~(1u << 30);
     default:
       // Nothing to do for an unsigned fixup.
       return EncodedValue;
     }
+  }
 
-
-  return EncodedValue & ~(1u << 30);
+  return EncodedValue;
 }
 
 void AArch64MCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                              SmallVectorImpl<MCFixup> &Fixups,
                                              const MCSubtargetInfo &STI) const {
   verifyInstructionPredicates(MI,
                               computeAvailableFeatures(STI.getFeatureBits()));
 

llvm/test/MC/AArch64/basic-a64-diagnostics.s

 // CHECK-ERROR-NEXT:              ^
 // CHECK-ERROR-NEXT: error: invalid operand for instruction
 // CHECK-ERROR-NEXT:         subs x5, xzr, #0x456, lsl #12
 // CHECK-ERROR-NEXT:                  ^
 
         // MOV alias should not accept any fiddling
         mov x2, xsp, #123
         mov wsp, w27, #0xfff, lsl #12
-// CHECK-ERROR: error: expected compatible register or logical immediate
+// CHECK-ERROR: error: invalid operand for instruction
 // CHECK-ERROR-NEXT:         mov x2, xsp, #123
-// CHECK-ERROR-NEXT:                 ^
+// CHECK-ERROR-NEXT:                      ^
 // CHECK-ERROR-NEXT: error: invalid operand for instruction
 // CHECK-ERROR-NEXT:         mov wsp, w27, #0xfff, lsl #12
 // CHECK-ERROR-NEXT:                       ^
 
         // A relocation should be provided for symbols
         add x3, x9, #variable
         add x3, x9, #variable-16
 // CHECK-ERROR: error: expected compatible register, symbol or integer in range [0, 4095]

llvm/test/MC/AArch64/mov-expression-as-immediate.s

+// RUN: llvm-mc -triple aarch64-none-linux-gnu %s -filetype=obj -o %t | llvm-objdump -d %t | FileCheck %s
+
+0:
+.skip 4
+1:
+mov x0, 1b - 0b
+// CHECK: mov x0, #4
+mov x0, 0b - 1b
+// CHECK: mov x0, #-4