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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
