[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

Backported to 9.0 in rL370181 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370073: [RISCV] Set MaxAtomicInlineWidth and 
MaxAtomicPromoteWidth for RV32/RV64… (authored by lenary, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57450?vs=217413=217418#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450

Files:
  cfe/trunk/lib/Basic/Targets/RISCV.h
  cfe/trunk/test/CodeGen/riscv-atomics.c
  cfe/trunk/test/Driver/riscv32-toolchain.c
  cfe/trunk/test/Driver/riscv64-toolchain.c

Index: cfe/trunk/lib/Basic/Targets/RISCV.h
===
--- cfe/trunk/lib/Basic/Targets/RISCV.h
+++ cfe/trunk/lib/Basic/Targets/RISCV.h
@@ -93,6 +93,13 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+MaxAtomicPromoteWidth = 128;
+
+if (HasA)
+  MaxAtomicInlineWidth = 32;
+  }
 };
 class LLVM_LIBRARY_VISIBILITY RISCV64TargetInfo : public RISCVTargetInfo {
 public:
@@ -110,6 +117,13 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+MaxAtomicPromoteWidth = 128;
+
+if (HasA)
+  MaxAtomicInlineWidth = 64;
+  }
 };
 } // namespace targets
 } // namespace clang
Index: cfe/trunk/test/CodeGen/riscv-atomics.c
===
--- cfe/trunk/test/CodeGen/riscv-atomics.c
+++ cfe/trunk/test/CodeGen/riscv-atomics.c
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -triple riscv32 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32I
+// RUN: %clang_cc1 -triple riscv32 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32IA
+// RUN: %clang_cc1 -triple riscv64 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64I
+// RUN: %clang_cc1 -triple riscv64 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64IA
+
+// This test demonstrates that MaxAtomicInlineWidth is set appropriately when
+// the atomics instruction set extension is enabled.
+
+#include 
+#include 
+
+void test_i8_atomics(_Atomic(int8_t) * a, int8_t b) {
+  // RV32I:  call zeroext i8 @__atomic_load_1
+  // RV32I:  call void @__atomic_store_1
+  // RV32I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV32IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV32IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV32IA: atomicrmw add i8* %a, i8 %b seq_cst
+  // RV64I:  call zeroext i8 @__atomic_load_1
+  // RV64I:  call void @__atomic_store_1
+  // RV64I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV64IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV64IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV64IA: atomicrmw add i8* %a, i8 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i32_atomics(_Atomic(int32_t) * a, int32_t b) {
+  // RV32I:  call i32 @__atomic_load_4
+  // RV32I:  call void @__atomic_store_4
+  // RV32I:  call i32 @__atomic_fetch_add_4
+  // RV32IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV32IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV32IA: atomicrmw add i32* %a, i32 %b seq_cst
+  // RV64I:  call signext i32 @__atomic_load_4
+  // RV64I:  call void @__atomic_store_4
+  // RV64I:  call signext i32 @__atomic_fetch_add_4
+  // RV64IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV64IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV64IA: atomicrmw add i32* %a, i32 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i64_atomics(_Atomic(int64_t) * a, int64_t b) {
+  // RV32I:  call i64 @__atomic_load_8
+  // RV32I:  call void @__atomic_store_8
+  // RV32I:  call i64 @__atomic_fetch_add_8
+  // RV32IA: call i64 @__atomic_load_8
+  // RV32IA: call void @__atomic_store_8
+  // RV32IA: call i64 @__atomic_fetch_add_8
+  // RV64I:  call i64 @__atomic_load_8
+  // RV64I:  call void @__atomic_store_8
+  // RV64I:  call i64 @__atomic_fetch_add_8
+  // RV64IA: load atomic i64, i64* %a seq_cst, align 8
+  // RV64IA: store atomic i64 %b, i64* %a seq_cst, align 8
+  // RV64IA: atomicrmw add i64* %a, i64 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
Index: cfe/trunk/test/Driver/riscv32-toolchain.c
===
--- cfe/trunk/test/Driver/riscv32-toolchain.c
+++ cfe/trunk/test/Driver/riscv32-toolchain.c
@@ -105,6 +105,10 @@
 typedef __SIZE_TYPE__ size_t;
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
+typedef __WINT_TYPE__ wint_t;
+
+
+// Check Alignments
 
 

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Alex Bradbury via Phabricator via cfe-commits]

asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary updated this revision to Diff 217413.
lenary added a comment.

Address review feedback:

- Add Test for MaxAtomicPromoteWidth


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450

Files:
  clang/lib/Basic/Targets/RISCV.h
  clang/test/CodeGen/riscv-atomics.c
  clang/test/Driver/riscv32-toolchain.c
  clang/test/Driver/riscv64-toolchain.c

Index: clang/test/Driver/riscv64-toolchain.c
===
--- clang/test/Driver/riscv64-toolchain.c
+++ clang/test/Driver/riscv64-toolchain.c
@@ -105,6 +105,10 @@
 typedef __SIZE_TYPE__ size_t;
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
+typedef __WINT_TYPE__ wint_t;
+
+
+// Check Alignments
 
 // CHECK: @align_c = dso_local global i32 1
 int align_c = __alignof(char);
@@ -118,6 +122,9 @@
 // CHECK: @align_wc = dso_local global i32 4
 int align_wc = __alignof(wchar_t);
 
+// CHECK: @align_wi = dso_local global i32 4
+int align_wi = __alignof(wint_t);
+
 // CHECK: @align_l = dso_local global i32 8
 int align_l = __alignof(long);
 
@@ -139,6 +146,88 @@
 // CHECK: @align_vl = dso_local global i32 8
 int align_vl = __alignof(va_list);
 
+// CHECK: @align_a_c = dso_local global i32 1
+int align_a_c = __alignof(_Atomic(char));
+
+// CHECK: @align_a_s = dso_local global i32 2
+int align_a_s = __alignof(_Atomic(short));
+
+// CHECK: @align_a_i = dso_local global i32 4
+int align_a_i = __alignof(_Atomic(int));
+
+// CHECK: @align_a_wc = dso_local global i32 4
+int align_a_wc = __alignof(_Atomic(wchar_t));
+
+// CHECK: @align_a_wi = dso_local global i32 4
+int align_a_wi = __alignof(_Atomic(wint_t));
+
+// CHECK: @align_a_l = dso_local global i32 8
+int align_a_l = __alignof(_Atomic(long));
+
+// CHECK: @align_a_ll = dso_local global i32 8
+int align_a_ll = __alignof(_Atomic(long long));
+
+// CHECK: @align_a_p = dso_local global i32 8
+int align_a_p = __alignof(_Atomic(void*));
+
+// CHECK @align_a_f = dso_local global i32 4
+int align_a_f = __alignof(_Atomic(float));
+
+// CHECK: @align_a_d = dso_local global i32 8
+int align_a_d = __alignof(_Atomic(double));
+
+// CHECK: @align_a_ld = dso_local global i32 16
+int align_a_ld = __alignof(_Atomic(long double));
+
+// CHECK: @align_a_s4 = dso_local global i32 4
+int align_a_s4 = __alignof(_Atomic(struct { char _[4]; }));
+
+// CHECK: @align_a_s8 = dso_local global i32 8
+int align_a_s8 = __alignof(_Atomic(struct { char _[8]; }));
+
+// CHECK: @align_a_s16 = dso_local global i32 16
+int align_a_s16 = __alignof(_Atomic(struct { char _[16]; }));
+
+// CHECK: @align_a_s32 = dso_local global i32 1
+int align_a_s32 = __alignof(_Atomic(struct { char _[32]; }));
+
+
+// Check Sizes
+
+// CHECK: @size_a_c = dso_local global i32 1
+int size_a_c = sizeof(_Atomic(char));
+
+// CHECK: @size_a_s = dso_local global i32 2
+int size_a_s = sizeof(_Atomic(short));
+
+// CHECK: @size_a_i = dso_local global i32 4
+int size_a_i = sizeof(_Atomic(int));
+
+// CHECK: @size_a_wc = dso_local global i32 4
+int size_a_wc = sizeof(_Atomic(wchar_t));
+
+// CHECK: @size_a_wi = dso_local global i32 4
+int size_a_wi = sizeof(_Atomic(wint_t));
+
+// CHECK: @size_a_l = dso_local global i32 8
+int size_a_l = sizeof(_Atomic(long));
+
+// CHECK: @size_a_ll = dso_local global i32 8
+int size_a_ll = sizeof(_Atomic(long long));
+
+// CHECK: @size_a_p = dso_local global i32 8
+int size_a_p = sizeof(_Atomic(void*));
+
+// CHECK: @size_a_f = dso_local global i32 4
+int size_a_f = sizeof(_Atomic(float));
+
+// CHECK: @size_a_d = dso_local global i32 8
+int size_a_d = sizeof(_Atomic(double));
+
+// CHECK: @size_a_ld = dso_local global i32 16
+int size_a_ld = sizeof(_Atomic(long double));
+
+
 // Check types
 
 // CHECK: define dso_local zeroext i8 @check_char()
Index: clang/test/Driver/riscv32-toolchain.c
===
--- clang/test/Driver/riscv32-toolchain.c
+++ clang/test/Driver/riscv32-toolchain.c
@@ -105,6 +105,10 @@
 typedef __SIZE_TYPE__ size_t;
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 typedef __WCHAR_TYPE__ wchar_t;
+typedef __WINT_TYPE__ wint_t;
+
+
+// Check Alignments
 
 // CHECK: @align_c = dso_local global i32 1
 int align_c = __alignof(char);
@@ -118,6 +122,9 @@
 // CHECK: @align_wc = dso_local global i32 4
 int align_wc = __alignof(wchar_t);
 
+// CHECK: @align_wi = dso_local global i32 4
+int align_wi = __alignof(wint_t);
+
 // CHECK: @align_l = dso_local global i32 4
 int align_l = __alignof(long);
 
@@ -139,6 +146,88 @@
 // CHECK: @align_vl = dso_local global i32 4
 int align_vl = __alignof(va_list);
 
+// CHECK: @align_a_c = dso_local global i32 1
+int align_a_c = __alignof(_Atomic(char));
+
+// CHECK: @align_a_s = dso_local global i32 2
+int align_a_s = __alignof(_Atomic(short));
+
+// CHECK: @align_a_i = dso_local global i32 4
+int align_a_i = __alignof(_Atomic(int));
+
+// CHECK: @align_a_wc = dso_local global i32 4
+int align_a_wc = 

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

This LGTM, but given how much discussion there has been about MaxPromoteWidth 
it would be great to get some test coverage for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

In D57450#1641308 , @jyknight wrote:

> In D57450#1641190 , @lenary wrote:
>
> > @jyknight I hear where you're coming from. I'll see what I can do about the 
> > psABI document.
> >
> > In that ticket, it's mentioned that the Darwin ABI explicitly says that 
> > non-power-of-two atomic types should be padded and realigned, but I cannot 
> > find any documentation explaining this. That would be useful, given 
> > presumably GCC does have to pad/align on Darwin.
>
>
> AFAIK, there is no such documentation, at least publicly. Possibly Apple has 
> some internally, but I suspect it more likely just some in-person 
> conversation or something.
>
> GCC is not really supported on Darwin, so I suspect it just gets it wrong.


To keep everyone updated, this has bubbled over into a thread on the GCC 
Mailing list: https://gcc.gnu.org/ml/gcc/2019-08/msg00191.html - Potentially 
this is a route to a resolution on non-RISC-V platforms (specifically x86).

> 
> 
>> Then the only outstanding question relates to zero-sized atomics, which GCC 
>> does not pad, but I think Clang has to pad to get the semantics correct, 
>> based on this comment: 
>> https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTContext.cpp#L2176
> 
> The semantics in GCC are that you can create such an object, but any attempt 
> to load or store it will result in a compile-time error. E.g., "error: 
> argument 1 of ‘__atomic_load’ must be a pointer to a nonzero size object". So 
> I don't think there's really an issue there.

Neat, ok.

I think the feeling with this ticket and the RISC-V backend is:

- We match GCC for power-of-two-sized atomics
- We don't match for non-power-of-two sized atomics (as on any other platform)

This feels like that should be enough for this patch to be merged, and in a 
future patch we can address the fall-out of ABIs changing across GCC and clang 
on more platforms than just RISC-V?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D57450#1641190 , @lenary wrote:

> @jyknight I hear where you're coming from. I'll see what I can do about the 
> psABI document.
>
> In that ticket, it's mentioned that the Darwin ABI explicitly says that 
> non-power-of-two atomic types should be padded and realigned, but I cannot 
> find any documentation explaining this. That would be useful, given 
> presumably GCC does have to pad/align on Darwin.


AFAIK, there is no such documentation, at least publicly. Possibly Apple has 
some internally, but I suspect it more likely just some in-person conversation 
or something.

GCC is not really supported on Darwin, so I suspect it just gets it wrong.

> Then the only outstanding question relates to zero-sized atomics, which GCC 
> does not pad, but I think Clang has to pad to get the semantics correct, 
> based on this comment: 
> https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTContext.cpp#L2176

The semantics in GCC are that you can create such an object, but any attempt to 
load or store it will result in a compile-time error. E.g., "error: argument 1 
of ‘__atomic_load’ must be a pointer to a nonzero size object". So I don't 
think there's really an issue there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

@jyknight I hear where you're coming from. I'll see what I can do about the 
psABI document.

In that ticket, it's mentioned that the Darwin ABI explicitly says that 
non-power-of-two atomic types should be padded and realigned, but I cannot find 
any documentation explaining this. That would be useful, given presumably GCC 
does have to pad/align on Darwin.

Then the only outstanding question relates to zero-sized atomics, which GCC 
does not pad, but I think Clang has to pad to get the semantics correct, based 
on this comment: 
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ASTContext.cpp#L2176


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

That GCC and Clang differ in handling of Atomics is a really unfortunate, 
longstanding issue.

See https://bugs.llvm.org/show_bug.cgi?id=26462

For RISCV, perhaps it's not yet too late to have the RISCV psABI actually 
specify a single ABI for C11 _Atomic which all compilers can implement, rather 
than having compilers do whatever they want to, separately...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

Cross linking to the relevant psABI pull request (still pending): 
https://github.com/riscv/riscv-elf-psabi-doc/pull/112


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary updated this revision to Diff 216594.
lenary added a comment.
Herald added a subscriber: pzheng.

Update MaxAtomicPromote width to treat it like an ABI feature, and set it to 128


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450

Files:
  clang/lib/Basic/Targets/RISCV.h
  clang/test/CodeGen/riscv-atomics.c

Index: clang/test/CodeGen/riscv-atomics.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv-atomics.c
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -triple riscv32 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32I
+// RUN: %clang_cc1 -triple riscv32 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32IA
+// RUN: %clang_cc1 -triple riscv64 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64I
+// RUN: %clang_cc1 -triple riscv64 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64IA
+
+// This test demonstrates that MaxAtomicInlineWidth is set appropriately when
+// the atomics instruction set extension is enabled.
+
+#include 
+#include 
+
+void test_i8_atomics(_Atomic(int8_t) * a, int8_t b) {
+  // RV32I:  call zeroext i8 @__atomic_load_1
+  // RV32I:  call void @__atomic_store_1
+  // RV32I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV32IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV32IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV32IA: atomicrmw add i8* %a, i8 %b seq_cst
+  // RV64I:  call zeroext i8 @__atomic_load_1
+  // RV64I:  call void @__atomic_store_1
+  // RV64I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV64IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV64IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV64IA: atomicrmw add i8* %a, i8 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i32_atomics(_Atomic(int32_t) * a, int32_t b) {
+  // RV32I:  call i32 @__atomic_load_4
+  // RV32I:  call void @__atomic_store_4
+  // RV32I:  call i32 @__atomic_fetch_add_4
+  // RV32IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV32IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV32IA: atomicrmw add i32* %a, i32 %b seq_cst
+  // RV64I:  call signext i32 @__atomic_load_4
+  // RV64I:  call void @__atomic_store_4
+  // RV64I:  call signext i32 @__atomic_fetch_add_4
+  // RV64IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV64IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV64IA: atomicrmw add i32* %a, i32 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i64_atomics(_Atomic(int64_t) * a, int64_t b) {
+  // RV32I:  call i64 @__atomic_load_8
+  // RV32I:  call void @__atomic_store_8
+  // RV32I:  call i64 @__atomic_fetch_add_8
+  // RV32IA: call i64 @__atomic_load_8
+  // RV32IA: call void @__atomic_store_8
+  // RV32IA: call i64 @__atomic_fetch_add_8
+  // RV64I:  call i64 @__atomic_load_8
+  // RV64I:  call void @__atomic_store_8
+  // RV64I:  call i64 @__atomic_fetch_add_8
+  // RV64IA: load atomic i64, i64* %a seq_cst, align 8
+  // RV64IA: store atomic i64 %b, i64* %a seq_cst, align 8
+  // RV64IA: atomicrmw add i64* %a, i64 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
Index: clang/lib/Basic/Targets/RISCV.h
===
--- clang/lib/Basic/Targets/RISCV.h
+++ clang/lib/Basic/Targets/RISCV.h
@@ -93,6 +93,13 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+MaxAtomicPromoteWidth = 128;
+
+if (HasA)
+  MaxAtomicInlineWidth = 32;
+  }
 };
 class LLVM_LIBRARY_VISIBILITY RISCV64TargetInfo : public RISCVTargetInfo {
 public:
@@ -110,6 +117,13 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+MaxAtomicPromoteWidth = 128;
+
+if (HasA)
+  MaxAtomicInlineWidth = 64;
+  }
 };
 } // namespace targets
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary planned changes to this revision.
lenary added a comment.

Upon further thought, I realise that `MaxAtomicPromoteWidth` should be set to 
128 regardless of whether a target `HasA`. I will be updating the patch with 
the new width and conditions early next week.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary commandeered this revision.
lenary added a reviewer: asb.
lenary added a comment.

Chatted to @asb and he wants me to take over this set of changes.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.
Herald added subscribers: s.egerton, MaskRay.

Given this is an ABI-compatibility issue, I've been looking at how GCC and 
Clang differ in how they deal with issues around size and alignment of atomic 
objects.

All types of size less than or equal to `MaxAtomicPromoteWidth`, when they are 
turned into their atomic variant, will: a) have their size rounded up to a 
power of two, and b) have their alignment changed to that same power of two.

The following Gist contains:

- a C program which will print the size and alignment of a variety of strangely 
sized and aligned objects, and their atomic variants.
- the output of this program when run through gcc and clang, specifying 
`rv(32,64)ima(,fd)` and `(ilp32,lp64)(,f)` (matching ABIs to architectures). I 
used `riscv64-unknown-linux-gnu-gcc` for the avoidance of doubt (this compiler 
allows you to compile for rv32 by specifying a 32-bit -march value).
- the diffs in those outputs for an single ABI, between the two different 
compilers.

The gist is here: 
https://gist.github.com/lenary/2e977a8af33876ba8e0605e98c4e1b0d

There are some differences between GCC and Clang, that seem not to be risc-v 
specific (I saw them when running the C program on my mac)

- Clang ensures zero-sized objects become char-sized when they become atomic. 
GCC leaves them zero-sized. This is documented in ASTContext.cpp line 2130, to 
ensure the atomic operation is generated.
- GCC seems not to round up the size and alignment of non-power-of-two-sized 
structures.

I think this patch needs to be updated to ensure there are no differences in 
objects of power-of-two size. To do this, both riscv64 and riscv32, should have 
a `MaxAtomicPromoteWidth` of 128.

@jyknight does this reasoning make sense for ABI compatibility?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[John LLVM via Phabricator via cfe-commits]

JohnLLVM added a comment.
Herald added subscribers: Jim, benna, psnobl, dexonsmith.

@asb: Gentle ping on this.

I would really like to avoid atomics libcalls in order to make proper llvm 
benchmarks.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: lib/Basic/Targets/RISCV.h:94
+if (HasA)
+  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 32;
+  }

MaxAtomicPromoteWidth is an incompatible ABI-change kind of thing.

We should set that to the maximum atomic size this target ABI will _EVER_ 
support with any hardware, since it changes the data layout for atomic types.

MaxAtomicInlineWidth can change based on hardware, and be increased in the 
future if other hardware is introduced, but MaxAtomicPromoteWidth shouldn't.

I think it should be set it to 64 for most 32-bit platforms, and 128 for most 
64-bit platforms.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[James Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Also worth noting that the frontend needs to know the max width otherwise 
`warn_atomic_op_misaligned` will be triggered with the message `large atomic 
operation may incur significant performance penalty` (name is misleading since 
the `%select{large|misaligned}0` means it's triggered for both large and 
misaligned atomic ops, and it's also in the `atomic-alignment` group). This is 
exactly what I did in my tree (other than the tests) and it seemed to work 
correctly for RV64A compiling FreeBSD.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D57450#1402153 , @rogfer01 wrote:

> Looks sensible to me.
>
> I'm just curious why we want to prevent emission of atomic LLVM instructions 
> at this point. Won't LLVM's AtomicExpand perform a similar lowering already? 
> Perhaps the goal is to save that pass some work?


There was some discussion about whether the frontend should lower to libcalls 
or not here https://bugs.llvm.org/show_bug.cgi?id=31620 and it seems the 
decided path was that it's better for the frontend to lower, even if 
AtomicExpandPass does have some support for introducing these libcalls. 
Additionally, setting the max atomic inline width should mean that 
`__atomic_is_lock_free` is evaluated correctly.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

Looks sensible to me.

I'm just curious why we want to prevent emission of atomic LLVM instructions at 
this point. Won't LLVM's AtomicExpand perform a similar lowering already? 
Perhaps the goal is to save that pass some work?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.
Herald added a project: clang.

Ping?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57450/new/

https://reviews.llvm.org/D57450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57450: [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Alex Bradbury via Phabricator via cfe-commits]

asb created this revision.
asb added reviewers: jfb, jyknight, wmi.
Herald added subscribers: jocewei, PkmX, rkruppe, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, 
niosHD, sabuasal, apazos, simoncook, johnrusso, rbar.

This ensures that libcalls aren't generated when the target supports atomics. 
Atomics aren't in the base RV32I/RV64I instruction sets, so 
MaxAtomicInlineWidth and MaxAtomicPromoteWidth are set only when the atomics 
extension is being targeted. This must be done in setMaxAtomicWidth, as this 
should be done after handleTargetFeatures has been called.


Repository:
  rC Clang

https://reviews.llvm.org/D57450

Files:
  lib/Basic/Targets/RISCV.h
  test/CodeGen/riscv-atomics.c

Index: test/CodeGen/riscv-atomics.c
===
--- /dev/null
+++ test/CodeGen/riscv-atomics.c
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -triple riscv32 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32I
+// RUN: %clang_cc1 -triple riscv32 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32IA
+// RUN: %clang_cc1 -triple riscv64 -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64I
+// RUN: %clang_cc1 -triple riscv64 -target-feature +a -O1 -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64IA
+
+// This test demonstrates that MaxAtomicInlineWidth is set appropriately when
+// the atomics instruction set extension is enabled.
+
+#include 
+#include 
+
+void test_i8_atomics(_Atomic(int8_t) * a, int8_t b) {
+  // RV32I:  call zeroext i8 @__atomic_load_1
+  // RV32I:  call void @__atomic_store_1
+  // RV32I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV32IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV32IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV32IA: atomicrmw add i8* %a, i8 %b seq_cst
+  // RV64I:  call zeroext i8 @__atomic_load_1
+  // RV64I:  call void @__atomic_store_1
+  // RV64I:  call zeroext i8 @__atomic_fetch_add_1
+  // RV64IA: load atomic i8, i8* %a seq_cst, align 1
+  // RV64IA: store atomic i8 %b, i8* %a seq_cst, align 1
+  // RV64IA: atomicrmw add i8* %a, i8 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i32_atomics(_Atomic(int32_t) * a, int32_t b) {
+  // RV32I:  call i32 @__atomic_load_4
+  // RV32I:  call void @__atomic_store_4
+  // RV32I:  call i32 @__atomic_fetch_add_4
+  // RV32IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV32IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV32IA: atomicrmw add i32* %a, i32 %b seq_cst
+  // RV64I:  call signext i32 @__atomic_load_4
+  // RV64I:  call void @__atomic_store_4
+  // RV64I:  call signext i32 @__atomic_fetch_add_4
+  // RV64IA: load atomic i32, i32* %a seq_cst, align 4
+  // RV64IA: store atomic i32 %b, i32* %a seq_cst, align 4
+  // RV64IA: atomicrmw add i32* %a, i32 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
+
+void test_i64_atomics(_Atomic(int64_t) * a, int64_t b) {
+  // RV32I:  call i64 @__atomic_load_8
+  // RV32I:  call void @__atomic_store_8
+  // RV32I:  call i64 @__atomic_fetch_add_8
+  // RV32IA: call i64 @__atomic_load_8
+  // RV32IA: call void @__atomic_store_8
+  // RV32IA: call i64 @__atomic_fetch_add_8
+  // RV64I:  call i64 @__atomic_load_8
+  // RV64I:  call void @__atomic_store_8
+  // RV64I:  call i64 @__atomic_fetch_add_8
+  // RV64IA: load atomic i64, i64* %a seq_cst, align 8
+  // RV64IA: store atomic i64 %b, i64* %a seq_cst, align 8
+  // RV64IA: atomicrmw add i64* %a, i64 %b seq_cst
+  __c11_atomic_load(a, memory_order_seq_cst);
+  __c11_atomic_store(a, b, memory_order_seq_cst);
+  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
+}
Index: lib/Basic/Targets/RISCV.h
===
--- lib/Basic/Targets/RISCV.h
+++ lib/Basic/Targets/RISCV.h
@@ -88,6 +88,11 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+if (HasA)
+  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 32;
+  }
 };
 class LLVM_LIBRARY_VISIBILITY RISCV64TargetInfo : public RISCVTargetInfo {
 public:
@@ -106,6 +111,11 @@
 }
 return false;
   }
+
+  void setMaxAtomicWidth() override {
+if (HasA)
+  MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+  }
 };
 } // namespace targets
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits