[PATCH] D56037: [AArch64] Emit the correct MCExpr relocations specifiers like VK_ABS_G0, etc

Mon Jan 7 11:32:32 PST 2019

efriedma added inline comments.

================
Comment at: lib/Target/AArch64/AArch64MCInstLower.cpp:212
+  else if ((MO.getTargetFlags() & AArch64II::MO_FRAGMENT) == AArch64II::MO_G0)
+    RefFlags |= AArch64MCExpr::VK_G0;
+
----------------
mgrang wrote:
> The problem with setting VK_NC (if MO_NC is set) is that a lot of unit tests fail with the following assert:
> 
> ```
>         adrp    x0, "??_C at foo@"
>         add     x0, x0, Invalid ELF symbol kind
> UNREACHABLE executed at /local/mnt/workspace/mgrang/comm/src/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCExpr.cpp:78!
> 
> ```
> 
> This is because a lot of flags like VK_PAGEOFF/VK_PAGE, etc also have to be set (like LowerSymbolELF does it). I am not sure that should be done in this patch.
> 
> Should I then restrict setting VK_NC only if one of MO_G3/MO_G2/MO_G1/MO_G0 and MO_NC are set?
Ultimately, we should only form an AArch64MCExprs where getKind() returns a known kind (one of the enum values from the list in AArch64MCExpr); otherwise, it's not clear what relocation is being requested.

I'm fine guarding it with "if (RefFlags)" or something like that for now (so we VK_NONE for cases you aren't addressing with this patch).  Please leave a FIXME; we should clean it up at some point for clarity, but it doesn't need to block this patch.

================
Comment at: lib/Target/AArch64/AArch64MCInstLower.cpp:233
+  AArch64MCExpr::VariantKind RefKind;
+  RefKind = static_cast<AArch64MCExpr::VariantKind>(RefFlags);
   Expr = AArch64MCExpr::create(Expr, RefKind, Ctx);
----------------
You can declare RefKind with type "auto" since it's obvious here.

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

https://reviews.llvm.org/D56037