Page MenuHomePhabricator

[Clang][LoongArch] Add inline asm support for constraints k/m/ZB/ZC
ClosedPublic

Authored by SixWeining on Sep 26 2022, 5:41 AM.

Details

Summary

Reference: https://gcc.gnu.org/onlinedocs/gccint/Machine-Constraints.html

k: A memory operand whose address is formed by a base register and
(optionally scaled) index register.

m: A memory operand whose address is formed by a base register and
offset that is suitable for use in instructions with the same
addressing mode as st.w and ld.w.

ZB: An address that is held in a general-purpose register. The offset
is zero.

ZC: A memory operand whose address is formed by a base register and
offset that is suitable for use in instructions with the same
addressing mode as ll.w and sc.w.

Note:
The INLINEASM SDNode flags in below tests are updated because the new
introduced enum Constraint_k is added before Constraint_m.

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll
llvm/test/CodeGen/X86/callbr-asm-kill.mir

This patch passes ninja check-all on a X86 machine with all official
targets and the LoongArch target enabled.

Diff Detail

Event Timeline

SixWeining created this revision.Sep 26 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 5:41 AM
SixWeining requested review of this revision.Sep 26 2022, 5:41 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 26 2022, 5:41 AM
xen0n accepted this revision.Sep 26 2022, 6:14 AM

The test cases look good to me. We should be pretty close to passing ninja check natively. Thanks!

This revision is now accepted and ready to land.Sep 26 2022, 6:14 AM
rengolin accepted this revision.Sep 27 2022, 2:40 PM

Looks good, with some nits. Thanks!

llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp
76

NIT: Use early exit.

if (ExtraCode)
  return false;
llvm/test/CodeGen/LoongArch/inline-asm-constraint-ZC.ll
17

Same comment as before, these CHECK lines look identical for both targets...

MaskRay added inline comments.Sep 27 2022, 3:20 PM
clang/lib/Basic/Targets/LoongArch.cpp
103

++Name

117

First std::string can be omitted

118

pre-increment

MaskRay added inline comments.Sep 27 2022, 3:21 PM
llvm/lib/Target/LoongArch/LoongArchISelDAGToDAG.cpp
92

Prefer initializing the variable when it is declared

113

Prefer initializing the variable when it is declared

SixWeining marked 4 inline comments as done.

Address comments from @rengolin and @MaskRay. Thanks.

clang/lib/Basic/Targets/LoongArch.cpp
117

Thanks.

llvm/lib/Target/LoongArch/LoongArchAsmPrinter.cpp
76

Thanks. But we should return true but not false.

llvm/test/CodeGen/LoongArch/inline-asm-constraint-ZC.ll
17

Unfortunately this is not the case becuase different instructions like add.w/add.d are used by LA32 and LA64 respectively.

Handle k and ZB in llvm/include/llvm/IR/InlineAsm.h::getMemConstraintName and add relevant tests.

Remove the first line "Assertions" in inline-asm-constraint-ZB.ll as it is not autogenerated.

This revision was landed with ongoing or failed builds.Sep 29 2022, 12:06 AM
This revision was automatically updated to reflect the committed changes.
MaskRay reopened this revision.Sep 29 2022, 12:55 AM
This revision is now accepted and ready to land.Sep 29 2022, 12:55 AM
MaskRay requested changes to this revision.Sep 29 2022, 12:55 AM
This revision now requires changes to proceed.Sep 29 2022, 12:55 AM
MaskRay added a comment.EditedSep 29 2022, 12:57 AM

Constraint_k somehow broke AArch64 and X86 inlineasm. Please run at least check-llvm check-clang for generic changes which may have farreaching effects to other targets.

The flag in the INLINEASM SDNode in llvm/test/CodeGen/X86/callbr-asm-kill.mir is updated because enum Constraint_k is added before Constraint_m.

SixWeining edited the summary of this revision. (Show Details)Sep 29 2022, 3:01 AM
SixWeining edited the summary of this revision. (Show Details)

Constraint_k somehow broke AArch64 and X86 inlineasm. Please run at least check-llvm check-clang for generic changes which may have farreaching effects to other targets.

Sorry for that. I have fix it. The fail is casued by a hard coded INLINEASM SDNode flag. I have updated it. Thanks.

xen0n added a comment.Sep 29 2022, 3:42 AM

The previous test results (with some of my WIP patches but unrelated to this) before the fix:

Failed Tests (6):
  LLVM :: CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll
  LLVM :: CodeGen/Generic/vector.ll
  LLVM :: CodeGen/PowerPC/2007-11-19-VectorSplitting.ll
  LLVM :: CodeGen/X86/callbr-asm-kill.mir
  LLVM :: DebugInfo/Generic/missing-abstract-variable.ll

Sure enough it was caught by my native build with all targets enabled, anyway it's important to at least enable the "big" targets such as X86 and AArch64 when configuring the local dev env. ;-)

update CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll and CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll

The previous test results (with some of my WIP patches but unrelated to this) before the fix:

Failed Tests (6):
  LLVM :: CodeGen/AArch64/GlobalISel/irtranslator-inline-asm.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/irtranslator-inline-asm.ll
  LLVM :: CodeGen/Generic/vector.ll
  LLVM :: CodeGen/PowerPC/2007-11-19-VectorSplitting.ll
  LLVM :: CodeGen/X86/callbr-asm-kill.mir
  LLVM :: DebugInfo/Generic/missing-abstract-variable.ll

Sure enough it was caught by my native build with all targets enabled, anyway it's important to at least enable the "big" targets such as X86 and AArch64 when configuring the local dev env. ;-)

Thanks for the tip. Just now I enabled all official targets and LoongArch and find 2 more fail tests (for same reason) for AArch64 and AMDGPU. I have fixed them and updated the patch.

Sorry again...

SixWeining edited the summary of this revision. (Show Details)Sep 29 2022, 4:57 AM
SixWeining edited the summary of this revision. (Show Details)Sep 29 2022, 5:33 PM
SixWeining added reviewers: foad, kschwarz.

A polite ping...

This revision was not accepted when it landed; it landed in state Needs Review.Oct 11 2022, 5:22 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.