[AArch64] Fix encoding for lsl #12 in add/sub immediates

[rovka]

Summary

Whenever an add/sub immediate needs a fixup, we set that immediate field to zero,
which is correct, but we also set the shift bits to zero, which is not true for
instructions that use lsl #12. This patch makes sure that if lsl #12 was used,
it will appear in the encoding of the instruction.

[rovka]

I checked with gas and it does the same thing for :tprel_hi12:, :lo12: and label arithmetic.

[peter.smith]

From an ELF perspective I think that this is the right thing to do. The only OpenSource ELF linker I know of that implements R_AARCH64_TLSLE_ADD_TPREL_HI12 does not touch the shift field when resolving the relocation. It is possible to write a test case that gives an incorrect result when there is a large enough amount of tls for the tprel_hi12 to be needed.

One thing I note that the GNU assembler will do is set the shift value if the result of the expression can be represented with the shift, but not without it. For example
.L1:
.space 0x10000
.L2:
From my reading of the code it seems like we'll fault this as out of range. If I'm right it would be nice to support this, but I don't think it necessarily needs to be done as part of this one.

lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp

266 ↗

(On Diff #69388)

Line 256 has an almost identical (ShiftVal == 0 ? 0 : (1 << 12)), it could be worth being consistent with this line although I don't have a particularly strong opinion.

[rengolin]

In D23930#528639, @peter.smith wrote:

From my reading of the code it seems like we'll fault this as out of range. If I'm right it would be nice to support this, but I don't think it necessarily needs to be done as part of this one.

Yes, this is an interesting enhancement, and should be reasonably simple. But not for this patch, I agree.

[rovka]

In D23930#528671, @rengolin wrote:

In D23930#528639, @peter.smith wrote:

From my reading of the code it seems like we'll fault this as out of range. If I'm right it would be nice to support this, but I don't think it necessarily needs to be done as part of this one.

Yes, this is an interesting enhancement, and should be reasonably simple. But not for this patch, I agree.

I don't know if it's that simple. The logic that actually computes the value to be stored in the immediate is part of the fixup handling, and at that point we're really only concerned with those 12 bits that represent the immediate (the shift bit is not part of the fixup). I can't think of a good way to handle this without having to extend the fixup to cover the shift bit as well, which is kind of awkward because then we'd have to express the shift as part of Expr (so we'd be creating a new shift expression with the original Expr as an operand, and then parsing that as part of the fixup). I'm not sure it's worth the effort.

Also, I think it would be difficult to maintain assembly that relies on this property: .space 0x1000 works, but .space 0x1000 - 4 gives an "immediate out of range" error, which just looks surprising unless you figure out to inspect the encodings.

Just my 2 cents.

[rengolin]

In D23930#528683, @rovka wrote:

Also, I think it would be difficult to maintain assembly that relies on this property: .space 0x1000 works, but .space 0x1000 - 4 gives an "immediate out of range" error, which just looks surprising unless you figure out to inspect the encodings.

You'd be surprised how some people use the assembler... :)

But that's for another patch.

I also agree with Peter. But would be good to get some Darwin perspective. @t.p.northover?

[rovka]

Ping?

[t.p.northover]

Seems basically reasonable. It would probably be better if we diagnosed attempts that ended up with a relocation (as far as I can see there's no good reason to write those forms). But since we don't do that now, it probably needn't be in this patch.

[rovka]

Thanks for having a look. Is it ok to commit? (With Peter's nit addressed)

[rengolin]

Sorry it took so long, this fell out of my radar. I think we're all in agreement. LGTM.

Thanks!
--renato

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

AArch64MCCodeEmitter::getAddSubImmOpValue(const MCInst &MI, unsigned OpIdx,
   const MCOperand &MO = MI.getOperand(OpIdx);
   const MCOperand &MO1 = MI.getOperand(OpIdx + 1);
   assert(AArch64_AM::getShiftType(MO1.getImm()) == AArch64_AM::LSL &&
          "unexpected shift type for add/sub immediate");
   unsigned ShiftVal = AArch64_AM::getShiftValue(MO1.getImm());
   assert((ShiftVal == 0 || ShiftVal == 12) &&
          "unexpected shift value for add/sub immediate");
   if (MO.isImm())
-    return MO.getImm() | (ShiftVal == 0 ? 0 : (1 << 12));
+    return MO.getImm() | (ShiftVal == 0 ? 0 : (1 << ShiftVal));
   assert(MO.isExpr() && "Unable to encode MCOperand!");
   const MCExpr *Expr = MO.getExpr();
 
   // Encode the 12 bits of the fixup.
   MCFixupKind Kind = MCFixupKind(AArch64::fixup_aarch64_add_imm12);
   Fixups.push_back(MCFixup::create(0, Expr, Kind, MI.getLoc()));
 
   ++MCNumFixups;
 
-  return 0;
+  return ShiftVal == 0 ? 0 : (1 << ShiftVal);
 }
 
 /// getCondBranchTargetOpValue - Return the encoded value for a conditional
 /// branch target.
 uint32_t AArch64MCCodeEmitter::getCondBranchTargetOpValue(
     const MCInst &MI, unsigned OpIdx, SmallVectorImpl<MCFixup> &Fixups,
     const MCSubtargetInfo &STI) const {
   const MCOperand &MO = MI.getOperand(OpIdx);

llvm/trunk/test/MC/AArch64/darwin-reloc-addsubimm.s

+// RUN: llvm-mc -triple=aarch64-darwin -filetype=obj %s -o - | \
+// RUN:   llvm-objdump -r -d - | FileCheck -check-prefix=OBJ %s
+
+// OBJ-LABEL: Disassembly of section __TEXT,__text:
+
+  add x2, x3, _data@pageoff
+// OBJ: [[addr:[0-9a-f]+]]: 62 00 00 91 add x2, x3, #0
+// OBJ-NEXT: [[addr]]: ARM64_RELOC_PAGEOFF12  _data
+
+  add x2, x3, #_data@pageoff, lsl #12
+// OBJ: [[addr:[0-9a-f]+]]: 62 00 40 91 add x2, x3, #0, lsl #12
+// OBJ-NEXT: [[addr]]: ARM64_RELOC_PAGEOFF12  _data

llvm/trunk/test/MC/AArch64/elf-reloc-addsubimm.s

 // RUN: llvm-mc -triple=aarch64-none-linux-gnu -filetype=obj %s -o - | \
-// RUN:   llvm-readobj -r | FileCheck -check-prefix=OBJ %s
+// RUN:   llvm-objdump -r -d - | FileCheck -check-prefix=OBJ %s
+
+// OBJ-LABEL: Disassembly of section .text:
 
         add x2, x3, #:lo12:some_label
+// OBJ: [[addr:[0-9a-f]+]]: 62 00 00 91 add x2, x3, #0
+// OBJ-NEXT: [[addr]]: R_AARCH64_ADD_ABS_LO12_NC  some_label
+
+        add x2, x3, #:lo12:some_label, lsl #12
+// OBJ: [[addr:[0-9a-f]+]]: 62 00 40 91 add x2, x3, #0, lsl #12
+// OBJ-NEXT: [[addr]]: R_AARCH64_ADD_ABS_LO12_NC  some_label
 
-// OBJ:      Relocations [
-// OBJ-NEXT:   Section {{.*}} .rela.text {
-// OBJ-NEXT:     0x0 R_AARCH64_ADD_ABS_LO12_NC some_label 0x0
-// OBJ-NEXT:   }
-// OBJ-NEXT: ]

llvm/trunk/test/MC/AArch64/tls-relocs.s

 
 // CHECK-ELF-NEXT:     0x38 R_AARCH64_TLSLD_MOVW_DTPREL_G0_NC [[VARSYM]]
 // CHECK-ELF-NEXT:     0x3C R_AARCH64_TLSLD_MOVW_DTPREL_G0_NC [[VARSYM]]
 
 
         add x17, x18, #:dtprel_hi12:var, lsl #12
         add w19, w20, #:dtprel_hi12:var, lsl #12
 
-// CHECK: add    x17, x18, :dtprel_hi12:var, lsl #12 // encoding: [0x51,0bAAAAAA10,0b00AAAAAA,0x91]
+// CHECK: add    x17, x18, :dtprel_hi12:var, lsl #12 // encoding: [0x51,0bAAAAAA10,0b01AAAAAA,0x91]
 // CHECK:                                            //   fixup A - offset: 0, value: :dtprel_hi12:var, kind: fixup_aarch64_add_imm12
-// CHECK: add    w19, w20, :dtprel_hi12:var, lsl #12 // encoding: [0x93,0bAAAAAA10,0b00AAAAAA,0x11]
+// CHECK: add    w19, w20, :dtprel_hi12:var, lsl #12 // encoding: [0x93,0bAAAAAA10,0b01AAAAAA,0x11]
 // CHECK:                                            //   fixup A - offset: 0, value: :dtprel_hi12:var, kind: fixup_aarch64_add_imm12
 
 // CHECK-ELF-NEXT:     0x40 R_AARCH64_TLSLD_ADD_DTPREL_HI12 [[VARSYM]]
 // CHECK-ELF-NEXT:     0x44 R_AARCH64_TLSLD_ADD_DTPREL_HI12 [[VARSYM]]
 
 
         add x21, x22, #:dtprel_lo12:var
         add w23, w24, #:dtprel_lo12:var

 
 // CHECK-ELF-NEXT:     0xC4 R_AARCH64_TLSLE_MOVW_TPREL_G0_NC [[VARSYM]]
 // CHECK-ELF-NEXT:     0xC8 R_AARCH64_TLSLE_MOVW_TPREL_G0_NC [[VARSYM]]
 
 
         add x17, x18, #:tprel_hi12:var, lsl #12
         add w19, w20, #:tprel_hi12:var, lsl #12
 
-// CHECK: add    x17, x18, :tprel_hi12:var, lsl #12 // encoding: [0x51,0bAAAAAA10,0b00AAAAAA,0x91]
+// CHECK: add    x17, x18, :tprel_hi12:var, lsl #12 // encoding: [0x51,0bAAAAAA10,0b01AAAAAA,0x91]
 // CHECK:                                           //   fixup A - offset: 0, value: :tprel_hi12:var, kind: fixup_aarch64_add_imm12
-// CHECK: add    w19, w20, :tprel_hi12:var, lsl #12 // encoding: [0x93,0bAAAAAA10,0b00AAAAAA,0x11]
+// CHECK: add    w19, w20, :tprel_hi12:var, lsl #12 // encoding: [0x93,0bAAAAAA10,0b01AAAAAA,0x11]
 // CHECK:                                           //   fixup A - offset: 0, value: :tprel_hi12:var, kind: fixup_aarch64_add_imm12
 
 // CHECK-ELF-NEXT:     0xCC R_AARCH64_TLSLE_ADD_TPREL_HI12 [[VARSYM]]
 // CHECK-ELF-NEXT:     0xD0 R_AARCH64_TLSLE_ADD_TPREL_HI12 [[VARSYM]]
 
 
         add x21, x22, #:tprel_lo12:var
         add w23, w24, #:tprel_lo12:var