This is an archive of the discontinued LLVM Phabricator instance.

[M68k] Support inline asm operands w/ simple constraints
ClosedPublic

Authored by myhsu on May 16 2021, 1:27 PM.

Details

Summary

This patch adds supports for inline assembly operands and some simple
operand constraints, including register and constant operands.

Diff Detail

Event Timeline

myhsu created this revision.May 16 2021, 1:27 PM
myhsu requested review of this revision.May 16 2021, 1:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 16 2021, 1:27 PM

LGTM but please review the linter warnings.

clang/lib/Basic/Targets/M68k.cpp
145

did you mean to drop support for 'f'?

llvm/test/CodeGen/M68k/inline-asm.ll
37

You probably should add tests for all newly added constraints with valid input.

myhsu updated this revision to Diff 346338.May 18 2021, 10:16 PM
myhsu marked 2 inline comments as done.

Fixed lint warning and added more test cases

clang/lib/Basic/Targets/M68k.cpp
145

To drop it for now. I believe this constraint was added by mistake since we barely has any support for floating point in the backend. I think it's a good idea to remove this constraint from the frontend so that it will not be misused until floating point implementation got matured.

Looking good @myhsu . Also, I got your LLVM book recently! You'll need to sign it for me at the next llvm conf in person.

clang/lib/Basic/Targets/M68k.cpp
197–199
return std::string("^") + std::string(Constraint++, 2);
clang/test/Sema/inline-asm-validate-m68k.c
10

Tests look good. Would you mind please adding an asm for each function that doesn't require an expected-error? ie. is valid input?

Also, volatile keyword is not necessary when there are no output operands. See also: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile.

51

do we need tests for lowercase a, r, or d?

llvm/lib/Target/M68k/M68kAsmPrinter.cpp
57–59

If DL is only needed for this lone case, consider using {} for the case and sinking the def closer to the use.

myhsu updated this revision to Diff 346638.May 19 2021, 11:20 PM
myhsu marked 4 inline comments as done.
  • Add test cases for valid constraints
  • Fix formatting

Looking good @myhsu . Also, I got your LLVM book recently! You'll need to sign it for me at the next llvm conf in person.

Thank you :-) Hope you will like it.
I'll start to practice my autograph too.

clang/test/Sema/inline-asm-validate-m68k.c
51

Actually I think r is already a built-in constraint letter for register. I'm adding test cases for a and d though.

nickdesaulniers accepted this revision.May 20 2021, 10:54 AM
This revision is now accepted and ready to land.May 20 2021, 10:54 AM
This revision was landed with ongoing or failed builds.May 20 2021, 2:01 PM
This revision was automatically updated to reflect the committed changes.