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