Fix LoongArch support #4652

heiher · 2024-05-09T12:14:49Z

This PR fixes support for LoongArch so that it can run on LoongArch systems.

gen/abi/loongarch64.cpp

runtime/druntime/src/core/threadasm.S

heiher · 2024-05-14T07:39:18Z

gen/abi/loongarch64.cpp

+
+    Type *ty = arg.type->toBasetype();
+    if (ty->isintegral() && (ty->ty == TY::Tint32 || ty->ty == TY::Tuns32)) {
+      arg.attrs.addAttribute(LLAttribute::SExt);


Refer: https://github.com/loongson/la-abi-specs/blob/release/lapcs.adoc

One exception to the above rule is that in the LP64D ABI, unsigned words, such as those representing unsigned int in C, are stored in general-purpose registers as proper sign extensions of their 32-bit values.

Please add a source comment, I was pretty sure this has got to be wrong.

I'll add a source comment and describe more here.

For example:

C

int32.c: (https://godbolt.org/z/vcjErxj76)

int check_int(int v) { return v; } unsigned int check_uint(unsigned int v) { return v; }

llvm ir:

define dso_local noundef signext i32 @check_int(i32 noundef returned signext %0) local_unnamed_addr #0 !dbg !9 { ... } define dso_local noundef signext i32 @check_uint(i32 noundef returned signext %0) local_unnamed_addr #0 !dbg !18 { ... }

D

int32.d:

extern(C) int check_int(int v) { return v; } extern(C) uint check_uint(uint v) { return v; }

llvm ir without this patch:

define i32 @check_int(i32 returned %v_arg) local_unnamed_addr #0 { ... } define i32 @check_uint(i32 returned %v_arg) local_unnamed_addr #0 { ... }

llvm ir with this patch:

define signext i32 @check_int(i32 returned signext %v_arg) local_unnamed_addr #0 { ... } define signext i32 @check_uint(i32 returned signext %v_arg) local_unnamed_addr #0 { ... }

Note that there's also TY::Tdchar, which is a 32-bit integer too.

Really. The corresponding type char32_t in C/C++ is also sign-extended. Thanks.

heiher · 2024-05-20T02:03:46Z

@kinke gentle ping.

kinke · 2024-05-21T22:00:18Z

gen/abi/loongarch64.cpp

+  // recursively visit a POD struct to flatten it
+  // FIXME: may cause low performance
+  // dmd may cache argtypes in some other architectures as a TypeTuple, but we
+  // need to additionally store field offsets to realign later


Could you please give an example of such a problematic struct, where the layout doesn't match?

Also note that a union would have overlapping fields. Official ABI docs often don't explicitly detail how unions are to be dealt with.

And stylistically, it looks like you never need the original ty, only ty->toBasetype(), so flattening it once at the beginning might make sense.

For example:

C

f32x2.c: (https://godbolt.org/z/vK599hrq7)

struct f32x2_s { float a; float b; }; struct f32x2_s f_f32x2_s(struct f32x2_s x) { x.a += 1.0; x.b += 1.0; return x; }

llvm ir:

define dso_local { float, float } @f_f32x2_s(float %0, float %1) local_unnamed_addr #0 !dbg !9 { ... }

D

f32x2.d:

extern(C) struct f32x2_s { float a; float b; } extern(C) f32x2_s f_f32x2_s(f32x2_s x) { x.a += 1.0; x.b += 1.0; return x; }

llvm ir without this patch:

define i64 @f_f32x2_s(i64 %x_arg) local_unnamed_addr #0 { ... }

llvm ir with this patch:

define { float, float } @f_f32x2_s({ float, float } %x_arg) local_unnamed_addr #0 { ... }

I meant an example where the struct needing a rewrite has a different layout than the rewritten one. Where you need to store the field offsets and do the piecewise memcpy's, instead of a trivial bitcast. This struct here wouldn't need any rewrite at all.

The code is copied from RISC-V, and I don't know which struct need to be rewritten to use temporary memory space to realign. Let's simplify it unless there are new tips.

The code is copied from RISC-V

Oh I see! Well if there'd be no need to ever 're-align', only bitcast, then a frontend toArgTypes() implementation would be much preferred, see e.g. https://github.com/ldc-developers/ldc/blob/master/dmd/argtypes_aarch64.d. That takes care of caching this info per type, makes it available for the va_arg implementation in druntime too, and also makes it usable for other compilers.

Thanks for your review and help. I agree that the front-end implementation would be a much better approach. I'm a newbie in D, so this might take some time for me. It would be best if we could move forward with the current patch to make things better, and I will continue to improve it.

Okay, I did one more review round, just some little nits.

kinke · 2024-05-21T22:06:08Z

gen/abi/loongarch64.cpp

+
+bool requireHardfloatRewrite(Type *ty) {
+  if (!ty->toBasetype()->isTypeStruct())
+    return false;


This excludes complex types and static arrays.

And I suggest adding the isPOD check here (and probably a size check to avoid costly flattening for larger types), instead of doing that at every call site.

The caller has done an isPOD check before calling requireHardfloatRewrite.

That's what I'm saying, it's ugly to do that at every call site, instead of once here and having the guarantee for the following logic then.

kinke · 2024-05-21T22:08:26Z

gen/abi/loongarch64.cpp

@@ -46,11 +188,23 @@ struct LoongArch64TargetABI : TargetABI {

  void rewriteFunctionType(IrFuncTy &fty) override {
    if (!fty.ret->byref) {
-      rewriteArgument(fty, *fty.ret);
+      if (!skipReturnValueRewrite(fty)) {
+        if (!fty.ret->byref && isPOD(fty.ret->type) &&


!fty.ret->byref is checked 3 times here; skipReturnValueRewrite() checks it too.

heiher

Thank you.

heiher · 2024-05-22T06:07:36Z

gen/abi/loongarch64.cpp

+
+bool requireHardfloatRewrite(Type *ty) {
+  if (!ty->toBasetype()->isTypeStruct())
+    return false;


The caller has done an isPOD check before calling requireHardfloatRewrite.

heiher · 2024-05-22T06:07:43Z

gen/abi/loongarch64.cpp

+  // recursively visit a POD struct to flatten it
+  // FIXME: may cause low performance
+  // dmd may cache argtypes in some other architectures as a TypeTuple, but we
+  // need to additionally store field offsets to realign later


For example:

C

f32x2.c: (https://godbolt.org/z/vK599hrq7)

struct f32x2_s { float a; float b; }; struct f32x2_s f_f32x2_s(struct f32x2_s x) { x.a += 1.0; x.b += 1.0; return x; }

llvm ir:

define dso_local { float, float } @f_f32x2_s(float %0, float %1) local_unnamed_addr #0 !dbg !9 { ... }

D

f32x2.d:

extern(C) struct f32x2_s { float a; float b; } extern(C) f32x2_s f_f32x2_s(f32x2_s x) { x.a += 1.0; x.b += 1.0; return x; }

llvm ir without this patch:

define i64 @f_f32x2_s(i64 %x_arg) local_unnamed_addr #0 { ... }

llvm ir with this patch:

define { float, float } @f_f32x2_s({ float, float } %x_arg) local_unnamed_addr #0 { ... }

heiher · 2024-05-22T06:08:14Z

gen/abi/loongarch64.cpp

@@ -46,11 +188,23 @@ struct LoongArch64TargetABI : TargetABI {

  void rewriteFunctionType(IrFuncTy &fty) override {
    if (!fty.ret->byref) {
-      rewriteArgument(fty, *fty.ret);
+      if (!skipReturnValueRewrite(fty)) {
+        if (!fty.ret->byref && isPOD(fty.ret->type) &&


heiher · 2024-05-22T06:11:59Z

gen/abi/loongarch64.cpp

+
+    Type *ty = arg.type->toBasetype();
+    if (ty->isintegral() && (ty->ty == TY::Tint32 || ty->ty == TY::Tuns32)) {
+      arg.attrs.addAttribute(LLAttribute::SExt);


I'll add a source comment and describe more here.

For example:

C

int32.c: (https://godbolt.org/z/vcjErxj76)

int check_int(int v) { return v; } unsigned int check_uint(unsigned int v) { return v; }

llvm ir:

define dso_local noundef signext i32 @check_int(i32 noundef returned signext %0) local_unnamed_addr #0 !dbg !9 { ... } define dso_local noundef signext i32 @check_uint(i32 noundef returned signext %0) local_unnamed_addr #0 !dbg !18 { ... }

D

int32.d:

extern(C) int check_int(int v) { return v; } extern(C) uint check_uint(uint v) { return v; }

llvm ir without this patch:

define i32 @check_int(i32 returned %v_arg) local_unnamed_addr #0 { ... } define i32 @check_uint(i32 returned %v_arg) local_unnamed_addr #0 { ... }

llvm ir with this patch:

define signext i32 @check_int(i32 returned signext %v_arg) local_unnamed_addr #0 { ... } define signext i32 @check_uint(i32 returned signext %v_arg) local_unnamed_addr #0 { ... }

kinke · 2024-05-23T11:27:24Z

gen/abi/loongarch64.cpp


 public:
  auto returnInArg(TypeFunction *tf, bool) -> bool override {
    if (tf->isref()) {
      return false;
    }
    Type *rt = tf->next->toBasetype();
+    if (!rt->size()) {
+      return false;
+    }


This isn't really required. Every non-POD has a non-empty size (empty structs have a size of 1 - it's only void, noreturn and 0-length static arrays that have a size of 0).

kinke · 2024-05-23T11:27:40Z

gen/abi/loongarch64.cpp

@@ -38,19 +133,30 @@ struct LoongArch64TargetABI : TargetABI {
  }

  auto passByVal(TypeFunction *, Type *t) -> bool override {
+    if (!t->size()) {
+      return false;
+    }


kinke · 2024-05-23T11:28:26Z

gen/abi/loongarch64.cpp

    }

    for (auto arg : fty.args) {
-      if (!arg->byref) {
+      if (!arg->byref && requireHardfloatRewrite(arg->type)) {


This now means that if arg->byref is true, rewriteArgument() is called, which is wrong.

kinke reviewed May 9, 2024

View reviewed changes

gen/abi/loongarch64.cpp Show resolved Hide resolved

heiher marked this pull request as draft May 10, 2024 01:40

heiher force-pushed the master branch from f723332 to 8507b16 Compare May 11, 2024 09:28

heiher marked this pull request as ready for review May 11, 2024 09:28

heiher requested a review from kinke May 11, 2024 09:28

SixWeining reviewed May 14, 2024

View reviewed changes

runtime/druntime/src/core/threadasm.S Outdated Show resolved Hide resolved

runtime/druntime/src/core/threadasm.S Outdated Show resolved Hide resolved

heiher force-pushed the master branch 3 times, most recently from 969ccb2 to fae65b3 Compare May 14, 2024 07:38

heiher commented May 14, 2024

View reviewed changes

heiher force-pushed the master branch from fae65b3 to c7993df Compare May 14, 2024 07:52

kinke reviewed May 21, 2024

View reviewed changes

heiher commented May 22, 2024

View reviewed changes

heiher force-pushed the master branch 2 times, most recently from 0dd59cc to 2f9b60c Compare May 22, 2024 12:37

kinke reviewed May 23, 2024

View reviewed changes

heiher added 4 commits May 23, 2024 20:34

ABI: Fix calling convention for LoongArch

ddbe443

ABI: Update spec url for LoongArch

b1b7409

druntime: Remove unused stack slot to end exactly on the memory boundary

090c361

druntime: Fix backtraces in Fibers for LoongArch

60a5517

heiher force-pushed the master branch from 2f9b60c to 60a5517 Compare May 23, 2024 12:59

kinke merged commit c91e199 into ldc-developers:master May 23, 2024
18 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LoongArch support #4652

Fix LoongArch support #4652

heiher commented May 9, 2024

heiher May 14, 2024

kinke May 21, 2024

heiher May 22, 2024 •

edited

kinke May 23, 2024

heiher May 23, 2024

heiher commented May 20, 2024

kinke May 21, 2024

kinke May 21, 2024

heiher May 22, 2024

kinke May 22, 2024 •

edited

heiher May 22, 2024

kinke May 22, 2024

heiher May 22, 2024

kinke May 23, 2024

kinke May 21, 2024

kinke May 21, 2024

heiher May 22, 2024

kinke May 22, 2024

heiher May 22, 2024

kinke May 21, 2024

heiher May 22, 2024

heiher left a comment

heiher May 22, 2024

heiher May 22, 2024

heiher May 22, 2024

heiher May 22, 2024 •

edited

kinke May 23, 2024

kinke May 23, 2024

kinke May 23, 2024

Fix LoongArch support #4652

Fix LoongArch support #4652

Conversation

heiher commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiher May 22, 2024 • edited

Choose a reason for hiding this comment

C

D

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiher commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C

D

kinke May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C

D

Choose a reason for hiding this comment

heiher May 22, 2024 • edited

Choose a reason for hiding this comment

C

D

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiher May 22, 2024 •

edited

kinke May 22, 2024 •

edited

heiher May 22, 2024 •

edited