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

[Renato Golin via Phabricator via llvm-commits]

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