This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

kschwarz created this revision.Apr 16 2020, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 11:27 AM
paquette added inline comments.Apr 21 2020, 10:55 AM
llvm/include/llvm/CodeGen/GlobalISel/InlineAsmLowering.h
42

Doxygen formatting for Ops?

\p Ops

Also can you add Doxygen comments for Val and Constraint?

44
  • Can this return a bool? It may be useful to know if this succeeds or fails.
  • Can Constraint be a StringRef?
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
268

Missing newline in debug string.

421

Should this newline be here? clang-format?

471

Can you use a range-based for loop here?

kschwarz updated this revision to Diff 262629.May 7 2020, 6:04 AM
kschwarz marked 7 inline comments as done.

Address review comments

llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
471

I've removed the loop entirely, and return if the number of registers is > 1. In that case we have to unpack the value first, which was missing here.

arsenm added inline comments.May 8 2020, 8:13 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
451

Extra newline

460

Extra newline

473

Missing return false?

574–575

Changing the extension type based on the byte-sizedness doesn't really make sense to me

kschwarz marked an inline comment as done.May 8 2020, 1:53 PM
kschwarz added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
574–575

I admit I simply followed what is done in the legalizer for extending constants (https://reviews.llvm.org/D70922).

Do you have a suggestion how the condition should look like instead?

arsenm added inline comments.May 8 2020, 2:50 PM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
574–575

I think we generally just sign extend immediate operands like this. There's no legalization or register here, so you shouldn't even need to look at the LLT

kschwarz marked an inline comment as done.May 9 2020, 1:52 AM
kschwarz added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
574–575

Yes, but it looks like inline asm boolean constants should behave differently (https://reviews.llvm.org/D60224)?

kschwarz updated this revision to Diff 263146.May 11 2020, 4:35 AM
kschwarz marked 4 inline comments as done.

Formatting issues, add missing return false;

llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
451

Is there a way to catch these formatting issues with clang-format? It doesn't remove those superfluous newlines for me

arsenm added inline comments.May 11 2020, 5:05 PM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
451

I think it just doesn't implement this

574–575

That seems convoluted. That's at least checking explicitly for a bit width of 1, not isByteSized

kschwarz updated this revision to Diff 263426.May 12 2020, 6:59 AM
kschwarz marked an inline comment as done.

Change boolean constant extension logic to just query the type's bit width

arsenm accepted this revision.May 13 2020, 2:23 PM

LGTM

This revision is now accepted and ready to land.May 13 2020, 2:23 PM
This revision was automatically updated to reflect the committed changes.

I'll have a look into it now. Thanks for letting me know!

I did not handle direct memory operands that need to be "indirectified" in this patch.
https://reviews.llvm.org/D79955 adds an early return for now so we don't run into the assertion while working on supporting that feature.