This is an archive of the discontinued LLVM Phabricator instance.

[Clang][LoongArch] Add inline asm support for constraints f/l/I/K
ClosedPublic

Authored by SixWeining on Sep 18 2022, 10:56 PM.

Details

Summary

This patch adds support for constraints f, l, I, K according
to [1]. The remain constraints (k, m, ZB, ZC) will be added
later as they are a little more complex than the others.
f: A floating-point register (if available).
l: A signed 16-bit constant.
I: A signed 12-bit constant (for arithmetic instructions).
K: An unsigned 12-bit constant (for logic instructions).

For now, no need to support register alias (e.g. $a0) in llvm as
clang will correctly decode the usage of register name aliases into
their official names. And AFAIK, the not yet upstreamed rustc for
LoongArch will always use official register names (e.g. $r4).

[1] https://gcc.gnu.org/onlinedocs/gccint/Machine-Constraints.html

Event Timeline

SixWeining created this revision.Sep 18 2022, 10:56 PM
SixWeining requested review of this revision.Sep 18 2022, 10:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 18 2022, 10:56 PM
xen0n added a comment.Sep 21 2022, 7:25 AM

I don't know if the clang changes should be split into its own commit (or the title of this commit amended to mention [Clang] but I don't know if this is appropriate), but the rest looks reasonable.

llvm/test/CodeGen/LoongArch/inline-asm-constraint-error.ll
3

double dashes for --mtriple

Use double dashes for --mtriple.

I don't know if the clang changes should be split into its own commit (or the title of this commit amended to mention [Clang] but I don't know if this is appropriate), but the rest looks reasonable.

Yes, I ever thought about spliting the patches into 2 (clang+llvm). But I finally decide to use a single one becuse I think this may help others a bit to understand how these inline asm(constrainsts, ...) are used and translated from frontend to backend.

SixWeining retitled this revision from [LoongArch] Add inline asm support for constraints f/l/I/K to [Clang][LoongArch] Add inline asm support for constraints f/l/I/K.Sep 22 2022, 5:17 AM
rengolin added inline comments.Sep 22 2022, 6:31 AM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
1958

Can't you just clip the $, upper-case and match against some table-gen'd names?

2064

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.
llvm/test/CodeGen/LoongArch/inline-asm-constraint.ll
6

I'm not seeing differences between LA32 and LA64, is splitting the CHECK lines really necessary?

On some other tests, too...

Address @rengolin's comments.

  1. Clip the '$' from original constraint string and match table-gen'd names.
  2. Delete unnecessary check prefix in tests.
SixWeining added inline comments.Sep 22 2022, 9:02 PM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
1958

Yes, it can reduce much code. Thanks.

2064

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.

llvm/test/CodeGen/LoongArch/inline-asm-constraint.ll
6

Yes. Let me remove them. Thanks.

rengolin accepted this revision.Sep 23 2022, 7:14 AM

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. :)

llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
2064

Ah, excellent. And the error is good too, so looks good.

This revision is now accepted and ready to land.Sep 23 2022, 7:14 AM