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

[Lu Weining via Phabricator via llvm-commits]

SixWeining 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
----------------
rengolin wrote:
> Can't you just clip the `$`, upper-case and match against some table-gen'd names?
Yes, it can reduce much code. Thanks.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:2064
+        uint64_t CVal = C->getSExtValue();
+        if (isInt<16>(CVal))
+          Ops.push_back(
----------------
rengolin wrote:
> 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.
If the constant isn't in the right type, SelectionDAGBuilder will emit an error. There is a test `llvm/test/CodeGen/LoongArch/inline-asm-constraint-error.ll` checking this.
```
// CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
   8987       if (OpInfo.ConstraintType == TargetLowering::C_Immediate ||
   8988           OpInfo.ConstraintType == TargetLowering::C_Other) {
   8989         std::vector<SDValue> Ops;
   8990         TLI.LowerAsmOperandForConstraint(InOperandVal, OpInfo.ConstraintCode,
   8991                                           Ops, DAG);
   8992         if (Ops.empty()) {
   8993           if (OpInfo.ConstraintType == TargetLowering::C_Immediate)
   8994             if (isa<ConstantSDNode>(InOperandVal)) {
   8995               emitInlineAsmError(Call, "value out of range for constraint '" +
   8996                                            Twine(OpInfo.ConstraintCode) + "'");
   8997               return;
   8998             }
```


For arm/aarch64, if the input is invalid, the function also directly return.
```
// AArch64ISelLowering.cpp
   9583     case 'K':
   9584       if (AArch64_AM::isLogicalImmediate(CVal, 32))
   9585         break;
   9586       return;
   9587     case 'L':
   9588       if (AArch64_AM::isLogicalImmediate(CVal, 64))
   9589         break;
   9590       return;
```

For LoongArch, `TargetLowering::LowerAsmOperandForConstraint` is only called when the constraint is not one of `l/I/K`.


================
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
+
----------------
rengolin wrote:
> I'm not seeing differences between LA32 and LA64, is splitting the CHECK lines really necessary?
> 
> On some other tests, too...
Yes. Let me remove them. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134157