This is an archive of the discontinued LLVM Phabricator instance.

[InlineAsm][AArch64]Add backend support for flag output parameters
ClosedPublic

Authored by mingmingl on Apr 23 2023, 5:27 PM.

Details

Summary

Before:

After:

  • Given flag output constraints in LLVM IR, condition code is parsed and flag output is lowered to 'cset'.
  • Clang parse is not added in this patch.

Diff Detail

Event Timeline

mingmingl created this revision.Apr 23 2023, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2023, 5:27 PM
mingmingl requested review of this revision.Apr 23 2023, 5:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2023, 5:27 PM
mingmingl updated this revision to Diff 516218.Apr 23 2023, 5:48 PM
mingmingl edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 24 2023, 2:54 PM

very nice, thank you!

(looks like https://reviews.llvm.org/rGe5c37958f901cc9bec50624dbee85d40143e4bca for inspiration)

Thanks for taking a look! Yep https://github.com/llvm/llvm-project/commit/e5c37958f901c is where I learned this. Going to port the frontend change (D57394) to aarch64 in a follow-up patch.

While testing D149123 with ./bin/clang -cc1 -S -triple=aarch64 inline-asm-aarch64-flag-output.c (which invokes global-isel for instruction selection according to print-after-all output), turns out GlobalISel doesn't support flag output (for x86 and aarch64) yet.

/bin/clang -cc1 -O1 -S -triple=aarch64 file.c and /bin/clang -cc1 -O2 -S -triple=aarch64 file.c works (gives the assembly output)

I filed https://github.com/llvm/llvm-project/issues/62343 to track but think the issue is a non-blocker of this patch and D149123 for now.

Adding a fallback from globalisel to selectiondag isel should be simple; I'd like to avoid advertising support for functionality that doesn't actually work at all optimization levels.

This is probably worth noting in release notes.

mingmingl planned changes to this revision.Apr 25 2023, 12:24 PM

Adding a fallback from globalisel to selectiondag isel should be simple;

Thanks for pointing out fallback mechanism (TargetLowering::fallBackToDAGISel)! Will do

I'd like to avoid advertising support for functionality that doesn't actually work at all optimization levels.

This is a valid point and sounds reasonable.

This is probably worth noting in release notes.

will do

Adding a fallback from globalisel to selectiondag isel should be simple; I'd like to avoid advertising support for functionality that doesn't actually work at all optimization levels.

This is probably worth noting in release notes.

mingmingl updated this revision to Diff 517003.EditedApr 25 2023, 5:52 PM

In llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp, allow Op ConstraintType to be Other. Add a test in 'llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll' to show that fallback works with -global-isel-abort=2

Please take another look. thanks!

This revision is now accepted and ready to land.Apr 25 2023, 5:52 PM

Adding a fallback from globalisel to selectiondag isel should be simple;

Thanks for pointing out fallback mechanism (TargetLowering::fallBackToDAGISel)! Will do

I'd like to avoid advertising support for functionality that doesn't actually work at all optimization levels.

This is a valid point and sounds reasonable.

This is probably worth noting in release notes.

will do

Adding a fallback from globalisel to selectiondag isel should be simple; I'd like to avoid advertising support for functionality that doesn't actually work at all optimization levels.

This is probably worth noting in release notes.

After spending some time on roughly figuring out how fallback works, relaxed the assertion as mentioned above and added a test to show that fallback works (with global-isel-abort=2 that enables fallback).

Currently global-isel is enabled for O0 with (the default) GlobalISelAbortMode GlobalISelAbort = GlobalISelAbortMode::Enable;, and explains the previous observation (O0 crashes and O1/O2 works).

So I'm thinking the change to 'clang/lib/Driver/ToolChains/Clang.cpp' (and release notes update) would goes to D149123

efriedma accepted this revision.Apr 25 2023, 6:54 PM

clang shouldn't abort by default, as far as I know?

Anyway, this patch looks fine.

clang shouldn't abort by default, as far as I know?

You are correct. After relaxing the assert condition in GlobalISel/InlineAsmLowering.cpp, the crashing O0 command generates valid output, indicating the abort option is not on -> after digging a little bit, the aarch64 target machine turns abort off. So no change needed in 'clang/lib/Driver/ToolChains/Clang.cpp'.

Anyway, this patch looks fine.

Thanks for reviews!

This revision was landed with ongoing or failed builds.Apr 26 2023, 9:45 AM
This revision was automatically updated to reflect the committed changes.