[PATCH] D134157: [Clang][LoongArch] Add inline asm support for constraints f/l/I/K
Renato Golin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 23 07:14:44 PDT 2022
rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.
Thanks, looks good to me!
I wouldn't split this in two commits. One of the benefits of having a monorepo is that we don't have to do that anymore. :)
================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:2064
+ uint64_t CVal = C->getSExtValue();
+ if (isInt<16>(CVal))
+ Ops.push_back(
----------------
SixWeining wrote:
> 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`.
Ah, excellent. And the error is good too, so looks good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134157/new/
https://reviews.llvm.org/D134157
More information about the llvm-commits
mailing list