[PATCH] D134157: [Clang][LoongArch] Add inline asm support for constraints f/l/I/K

[Renato Golin via Phabricator via cfe-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:1958
+  // constraints while the official register name is prefixed with a '$'.
+  // So we manually select general purpose registers here.
+  // For now, no need to support ABI names (e.g. `$a0`) as clang will correctly
----------------
Can't you just clip the `$`, upper-case and match against some table-gen'd names?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:2064
+        uint64_t CVal = C->getSExtValue();
+        if (isInt<16>(CVal))
+          Ops.push_back(
----------------
Shouldn't this break if the constant isn't in the right type? What happens if it isn't? 

It seems it just doesn't append the operand and return. Wouldn't that just break the op's format?

On Arm, the logic is simpler:
 - Iterate through all constraints, validating the input
 - If valie, set the Result and append to the Op at the end
 - Otherwise bail and let `TargetLowering::LowerAsmOperandForConstraint` handle it.


================
Comment at: llvm/test/CodeGen/LoongArch/inline-asm-constraint.ll:5
+; RUN: llc --mtriple=loongarch64 --verify-machineinstrs --no-integrated-as < %s \
+; RUN:   | FileCheck --check-prefix=LA64 %s
+
----------------
I'm not seeing differences between LA32 and LA64, is splitting the CHECK lines really necessary?

On some other tests, too...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134157