[PATCH] D149123: [AArch64][InlineAsm]Add Clang support for flag output constraints

[Mingming Liu via Phabricator via cfe-commits]

mingmingl added a comment.

In D149123#4294619 <https://reviews.llvm.org/D149123#4294619>, @mingmingl wrote:

> While testing this patch 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 yet (for x86 or aarch64).
>
> `/bin/clang -cc1 -O1 -S -triple=aarch64 file.c` and `/bin/clang -cc1 -O2 -S -triple=aarch64 file.c` works.
>
> I filed https://github.com/llvm/llvm-project/issues/62343 to track but but think the issue is a non-blocker of this patch and D149032 <https://reviews.llvm.org/D149032> for now.

After relaxing the assert condition in GlobalISel/InlineAsmLowering.cpp in the parent patch, the original crash (assertion error in global-isel) command generates valid asm outputs (the reason is that global-isel will fall back <https://github.com/llvm/llvm-project/blob/1e4de2d2701633a97de96c7a55353879ec9b7aa4/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp#L365> to selection-dag based isel when it couldn't select or lower an instruction), The error seen in issue 62343 could be worked around with the fallback approach.

Mentioned this change in clang release notes, and resolve comments.



================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:1216
+// Returns the length of cc constraint.
+static unsigned matchAsmCCConstraint(const char *&Name) {
+  constexpr unsigned len = 5;
----------------
nickdesaulniers wrote:
> davidxl wrote:
> > Name is not modified in this method, so perhaps dropping '&'?
> Yeah, looks like this was copied from D57394. Probably both places should be fixed.
> 
> A `char *&` is a code smell.
thanks for the catch! will fix the x86 cpp code in a tiny separate patch later.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:1310
+    // CC condition
+    if (auto Len = matchAsmCCConstraint(Name)) {
+      Name += Len - 1;
----------------
nickdesaulniers wrote:
> please don't use `auto` here.
s/auto/const unsiged, here and above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149123/new/

https://reviews.llvm.org/D149123