This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support global address as inline asm memory operand of `m`
ClosedPublic

Authored by wangpc on Aug 14 2023, 1:06 AM.

Details

Summary

In D146245, we have supported lowering inline asm m with offset
to register+imm, but we didn't handle the case that the offset
is the low part of global address.

This patch will emit %lo(g) when g is a global address.

Fixes #64656

Diff Detail

Unit TestsFailed

Event Timeline

wangpc created this revision.Aug 14 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 1:06 AM
wangpc requested review of this revision.Aug 14 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 1:06 AM
wangpc added inline comments.Aug 14 2023, 1:08 AM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
246

Is this the right way to print this operand?

wangpc updated this revision to Diff 549820.Aug 14 2023, 1:20 AM

Use getName().

asb accepted this revision.Aug 14 2023, 6:59 AM

LGTM, though I'd appreciate an extra pair of eyes that there aren't other lurking problems that have been missed.

Could you please expand the test cases a bit? It seems worth testing e.g.

@ga1 = dso_local global [4000 x i32] zeroinitializer, align 4
...
  call void asm "sw zero, $0", "=*m"(ptr nonnull elementtype(i32) getelementptr inbounds ([400000 x i32], ptr @ga1, i32 0, i32 2000))

and likely a variant with a smaller offset too. Both seem to generate reasonable output to my eye.

This revision is now accepted and ready to land.Aug 14 2023, 6:59 AM

Confirmed this match GCC behavior, LGTM too,

wangpc updated this revision to Diff 550193.Aug 14 2023, 10:19 PM

Add more tests.

medany tests for %pcrel_lo12 being handled correctly?

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.

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.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
246

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.

wangpc updated this revision to Diff 550264.Aug 15 2023, 4:46 AM
  • Use lowerOperand()
  • Test PIC code.
wangpc updated this revision to Diff 550308.Aug 15 2023, 6:32 AM
wangpc marked an inline comment as done.

Rename DispImm to Offset.

wangpc updated this revision to Diff 550584.Aug 15 2023, 8:36 PM

Update tests.

asb accepted this revision.Aug 21 2023, 1:37 AM

I think all outstanding questions / requests were resolved, so marking as LGTM again.

This revision was landed with ongoing or failed builds.Aug 21 2023, 4:00 AM
This revision was automatically updated to reflect the committed changes.