This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Teach RISCVMergeBaseOffset to handle inline asm
Needs ReviewPublic

Authored by wangpc on Aug 16 2023, 2:52 AM.

Details

Summary

For inline asm with memory operands, we can merge the offset into
the second operand of memory constraint operands.

Diff Detail

Event Timeline

wangpc created this revision.Aug 16 2023, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 2:52 AM
wangpc requested review of this revision.Aug 16 2023, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 2:52 AM

There is a problem here, can someone explain it?

For LLVM IR:
reduced.ll

@g = external global [4000 x i32], align 4
define void @test() nounwind {
  call void asm "sw zero, $0", "=*m"(ptr elementtype(i32) @g)
  ret void
}

llc -code-model=medium -no-integrated-as reduced.ll

test:                                   # @test
# %bb.0:
.Lpcrel_hi0:
	auipc	a0, %pcrel_hi(g)
	#APP
	sw zero, %pcrel_lo(.Lpcrel_hi0)(a0)
	#NO_APP
	ret

llc -code-model=medium reduced.ll

test:                                   # @test
# %bb.0:
.Lpcrel_hi0:
	auipc	a0, %pcrel_hi(g)
	#APP
	sw	zero, %pcrel_lo(.Lpcrel_hi00)(a0)
	#NO_APP
	ret

The difference is that there is an extra 0 after .Lpcrel_hi when there is no -no-integrated-as.
Is this a bug or a feature by design?

craig.topper added inline comments.Aug 16 2023, 10:27 AM
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
402

What about INLINEASM_BR?

431

If there are multiple INLINE_ASM instructions, would InlineAsmMemoryOpIndexes need to know which instruction the index belong to?

craig.topper added inline comments.Aug 16 2023, 10:35 AM
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
475

Can we use MachineOperand::ChangeToGA and MachineOperand::ChangeToMCSymbol to avoid building a new instruction?

craig.topper added inline comments.Aug 16 2023, 10:36 AM
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
475

Though I guess we would also need ChangeToCPI for constant pool?

wangpc updated this revision to Diff 551025.Aug 17 2023, 12:20 AM
  • Add INLINEASM_BR.
  • Use changeToXXX to update operands instead of creating new MI.
  • Record indexes of each inline asm MI.
wangpc marked 4 inline comments as done.Aug 17 2023, 12:28 AM
wangpc added inline comments.
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
475

There are four cases of using RISCVTargetLowering::getAddr: GlobalAddress, BlockAddress, ConstantPool and JumpTable.
BlockAddress, ConstantPool and JumpTable are used in compiler intenelly, so user can't use it in inline asm(?). And there is no corresponding changeToXXX methods for them (If they are really needed, I can add them in another patch).

wangpc updated this revision to Diff 551027.Aug 17 2023, 12:33 AM
wangpc marked an inline comment as done.

Use reference.

craig.topper added inline comments.Aug 23 2023, 4:15 PM
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
32

This is unused now.

408

Use a const reference

420

Is this auto making a copy of the operand? If so, we should use a const reference.

424

Same here

wangpc updated this revision to Diff 553355.Aug 24 2023, 10:13 PM
wangpc marked 4 inline comments as done.

Rebase and address comments.

Ping. Is it OK now?

This revision is now accepted and ready to land.Aug 30 2023, 11:32 PM

@samitolvanen is reporting that this is causing breakage for us in the Linux kernel: https://github.com/ClangBuiltLinux/linux/issues/1928.

Please consider whether this can be fixed forward quickly, or should be reverted. Otherwise I will revert shortly.

@samitolvanen is reporting that this is causing breakage for us in the Linux kernel: https://github.com/ClangBuiltLinux/linux/issues/1928.

Please consider whether this can be fixed forward quickly, or should be reverted. Otherwise I will revert shortly.

I think the problem is that atomic instructions don't have an immediate offset field so they are different than other instructions. I'm not sure we can generically do this patch for the "m" constraint. We should revert.

@samitolvanen is reporting that this is causing breakage for us in the Linux kernel: https://github.com/ClangBuiltLinux/linux/issues/1928.

Please consider whether this can be fixed forward quickly, or should be reverted. Otherwise I will revert shortly.

I think the problem is that atomic instructions don't have an immediate offset field so they are different than other instructions. I'm not sure we can generically do this patch for the "m" constraint. We should revert.

Doing it for m is fine. What's not fine is doing it for A, as used by Linux in the reported errors, which presumably presents itself close enough to m to not be caught by the code here. Presumably we need a constraint_A_with_global_1 test to catch this.

jrtc27 added inline comments.Aug 31 2023, 2:03 PM
llvm/lib/Target/RISCV/RISCVMergeBaseOffset.cpp
475

You can absolutely get a BlockAddress in IR, and that's exposed to C for the GNU indirect goto &&label syntax. You have to write some very cursed C (something like "m"(*(void **)&&label)) to end up with it in an m constraint, but it's technically possible to write, and so that input will now hit this report_fatal_error.

Sorry for the revert, but I've left two reduced test cases in https://github.com/ClangBuiltLinux/linux/issues/1928. My hope is that those can help you reland this work.

Doing it for m is fine. What's not fine is doing it for A, as used by Linux in the reported errors, which presumably presents itself close enough to m to not be caught by the code here. Presumably we need a constraint_A_with_global_1 test to catch this.

Ah, good eye! That appeared in both reproducers linked above. Thanks for taking the time to analyze this and for your quick feedback @jrtc27 and @craig.topper .

craig.topper reopened this revision.Aug 31 2023, 2:23 PM
This revision is now accepted and ready to land.Aug 31 2023, 2:23 PM
craig.topper requested changes to this revision.Aug 31 2023, 2:23 PM
This revision now requires changes to proceed.Aug 31 2023, 2:23 PM
wangpc updated this revision to Diff 556997.Sep 18 2023, 10:38 PM
  • Rebase.
  • Don't optimize for constraint A.
wangpc marked an inline comment as done.Sep 18 2023, 10:40 PM

Ping.
Are there any more concerns? @jrtc27 @asb

One more ping.

One more ping.

US llvm dev meeting was this week. Sorry for delay. I’ll look when I can.

Ping.
(I have almost forgotten this patch)