[PATCH] D80028: [AArch64] Support expression results as immediate values in mov

[Jian Cai via Phabricator via llvm-commits]

jcai19 marked an inline comment as done.
jcai19 added inline comments.


================
Comment at: llvm/test/MC/AArch64/mov-invalid-expression-as-immediate.s:9
+// CHECK: mov x0, 1b - 0b
+// CHECK: ^
----------------
efriedma wrote:
> jcai19 wrote:
> > efriedma wrote:
> > > jcai19 wrote:
> > > > efriedma wrote:
> > > > > jcai19 wrote:
> > > > > > efriedma wrote:
> > > > > > > Please also add a testcase for something like `mov x0, 0b`, where the value can't be resolved in the assembler.
> > > > > > 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.
> > > > > 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.)
> > > > 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?
> > > > 
> > > 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.
> > 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.
> > 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.
Thank you for the all clarification! I have updated the patch to not create AAarch64MCExpr.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80028/new/

https://reviews.llvm.org/D80028