This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][InlineAsm] Add support for basic output operand constraints
ClosedPublic

Authored by kschwarz on Apr 16 2020, 11:24 AM.

Diff Detail

Event Timeline

kschwarz created this revision.Apr 16 2020, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 11:24 AM
arsenm added inline comments.Apr 16 2020, 1:16 PM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
90

Don't need to repeat function name

92

Start with lowercase

119–120

Picking types from classes is usually problematic for me. Do we really need this type, and if so, is it feasible to use LLT instead?

140

RC->contains()?

214

Start with lowercase

kschwarz updated this revision to Diff 258303.Apr 17 2020, 6:07 AM

Address review comments

kschwarz marked 6 inline comments as done.Apr 17 2020, 6:10 AM
kschwarz added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
119–120

Nope, I actually had a FIXME to remove it. We should only need the register size, and this we can always recompute.

arsenm added inline comments.Apr 17 2020, 6:50 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
105

This should probably propagate that this is an error with the function return value?

120

Specify physical register in comment?

128–131

Looking again this assert doesn't really make sense. You are finding the registers from the reg class iterator, so obviously it is in the class already

133

Picking multiple physical registers by scanning through the register class doesn't make any sense to me. I would expect NumRegs > 1 only with a virtual register

154

Move this into default case?

380

Reg.isPhysical()

kschwarz marked an inline comment as done.Apr 20 2020, 11:18 AM
kschwarz added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
120

Which comment do you mean?

128–131

I've mostly copied this function from the SelectionDAG code, but I agree this assertion should never trigger. I couldn't find out from the history why this assertion was added, maybe because the target provides both the physical register and the register class, and they have to match?
I'll remove the assertion in the next round.

133

The LLVM IR reference manual contains the following paragraph:

There is also an “interesting” feature which deserves a bit of explanation: if a register class constraint allocates a register which is too small for the value type operand provided as input, the input value will be split into multiple registers, and all of them passed to the inline asm.

X86 uses this at least with the 'A' register constraint, but there are other targets, too (e.g. ARM uses it to pass 64-bit values in even-odd register pairs).

154

At least my Xcode warns about adding a default case for a switch which covers all enumeration values. I think there's also a similar notice in the development guide?

kschwarz updated this revision to Diff 258943.Apr 21 2020, 2:54 AM
kschwarz marked 2 inline comments as done.

Address some review comments

arsenm added inline comments.Apr 21 2020, 7:26 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
133

With physical registers? Can you add a testcase for this?

kschwarz added inline comments.Apr 21 2020, 9:01 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
133

I can add a fictitious test for AArch64, but it doesn't have asm template argument modifiers to access the "higher" register.
ARM has test cases for this (for SelectionDAG though) here: https://github.com/llvm/llvm-project/blob/master/llvm/test/CodeGen/ARM/inlineasm-operand-implicit-cast.ll#L291

paquette added inline comments.Apr 21 2020, 11:13 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
118–123

Comments seem out of place here?

Move them closer to the code that corresponds to them?

128–131

Maybe just use std::find_if?

Also what should happen if you don't find the assigned register here?

179

Extra parens around if?

222–224

Remove braces on single-line if?

316

s/node/instruction/

355–359

Can this be a range-based for?

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
3

-verify-machineinstrs?

kschwarz updated this revision to Diff 259277.Apr 22 2020, 7:05 AM
kschwarz marked 8 inline comments as done.

Rebase & address more review comments

llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
128–131

Previously, there was an assertion if the end of the iterator was reached without finding the register, but I removed it in another iteration.
Since the physical register and the register class are both provided by the target, there isn't really anything else we can do than abort the compilation if they don't match. I'm adding back the assertion for this case then.

133

Turns out I can't add a test for this at the moment, because I'm bailing out if an output constraint needs more than one register (see line 408).
Do you want me to add support for it in this patch?

arsenm added inline comments.Apr 22 2020, 7:38 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
251

Add a TRI variable instead of calling getSubtarget().getRegisterInfo() repeatedly through the function

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
693–694

This part isn't tested, I would leave this for a separate patch. I'm not sure checking for inline asm explicitly is the right thing here

kschwarz marked an inline comment as done.Apr 22 2020, 8:37 AM
kschwarz added inline comments.
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
693–694

Sure I can remove this change from the patch. I temporarily removed the -stop-after argument from the irtranslator test to run down to assembly, which resulted in a crash in the register bank selector of AArch64.
Since we cannot have generic virtual registers on INLINEASM instructions, I figured not trying to select a register bank for these instructions would be fine.

kschwarz updated this revision to Diff 259848.Apr 24 2020, 3:41 AM
kschwarz marked 2 inline comments as done.

Hoist call to getRegisterInfo() and create local variable. Fix a few clangTidy issues

kschwarz added inline comments.Apr 24 2020, 3:42 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
693–694

After removing this line, another AArch64 test starts failing (stack_guard_remat.ll). It triggers the very same crash in the register bank selector of AArch64: llvm::LLT::getScalarSizeInBits() const: Assertion 'RawData != 0 && "Invalid Type"'.

What do you think would be an appropriate check during register bank selection?

arsenm added inline comments.Apr 24 2020, 3:54 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
693–694

OK, can you add an explicit regbankselect for asm test then?

kschwarz updated this revision to Diff 260247.Apr 27 2020, 2:24 AM
kschwarz marked 2 inline comments as done.

Add regbank-select test.

kschwarz added inline comments.Apr 27 2020, 2:26 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
693–694

I've added a test and extracted the MI.isInlineAsm() check similar to the MI.isDebugInstr() below.

Note, however, that I had to specifically add the register class annotations to the input MIR (for the virtual register definitions of inline asm instructions), because it is not printed by the MIRPrinter by default. This makes the current output of MIRPrinter for inline asm instructions not round-trippable.

@arsenm, is this good to go now?

arsenm accepted this revision.May 5 2020, 6:52 AM

LGTM

This revision is now accepted and ready to land.May 5 2020, 6:52 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/AArch64/GlobalISel/regbank-inlineasm.mir