[PATCH] D157839: [RISCV] Support global address as inline asm memory operand of `m`

Tue Aug 15 00:13:27 PDT 2023

jrtc27 added a comment.

In D157839#4587408 <https://reviews.llvm.org/D157839#4587408>, @wangpc wrote:

> In D157839#4587325 <https://reviews.llvm.org/D157839#4587325>, @jrtc27 wrote:
>
>> medany tests for %pcrel_lo12 being handled correctly?
>
> For medany code model, `TargetGlobalAddress` will be lowered to `LLA` or `LGA` pseudos, and `SelectAddrRegImm` in `SelectInlineAsmMemoryOperand` will return the `LLA` or `LGA` and offset `0`. The offset will always be 0, so I think it is handled correctly. And I just confirmed that GCC has the same behavior in medany code model.

That's not what I asked. I asked for tests, so please add them. What you say may be true today, but the point of tests isn't just to demonstrate that the code today does what you think it does, but also to ensure that the code tomorrow continues to do so. Otherwise someone could come along and alter how medany TGAs get lowered and end up exposing them to PrintAsmMemoryOperand without realising due to having no test coverage of it.

I'll also note that we //should// be able to fold the %pcrel_lo12 into the inline assembly, even if we don't do so today.

================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:246
+  else if (DispImm.isGlobal())
+    OS << "%lo(" << DispImm.getGlobal()->getGlobalIdentifier() << ")";
+  OS << "(" << RISCVInstPrinter::getRegisterName(AddrReg.getReg()) << ")";
----------------
wangpc wrote:
> Is this the right way to print this operand?
Should be not lower it to an MCOperand with lowerOperand (or lowerSymbolOperand if you want to keep the imm case separate) and let that do the printing rather than hand-rolling a subset of it? At least assert on the MachineOperand's target flags so we don't emit %lo for something that's not a lo.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157839