[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

@yaxunl I've addressed your remarks on a GitHub PR: 
https://github.com/llvm/llvm-project/pull/65938

Thanks for the remarks! It made me realize that I could simplify the code a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

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

[PATCH] D159257: [Clang] Drop functions with incompatible target-features when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez abandoned this revision.
jmmartinez added a comment.

Moved into https://github.com/llvm/llvm-project/pull/65737


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159257

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

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd60c47476dde: [Clang] Propagate target-features if 
compatible when using mlink-builtin-bitcode (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,49 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
 // RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
 
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s
+
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2030-2031
+  bool EnabledForTarget = TEntry->second;
+  if (EnabledForTarget != EnabledForFunc)
+return;
+}

arsenm wrote:
> jmmartinez wrote:
> > arsenm wrote:
> > > Early return breaks the other features
> > I did not understand this remark.
> > 
> > If the features are not compatible, we do not add a "target-features" entry 
> > in the new "FuncAttrs". Then, the old "target-features" entry is kept in 
> > the Function coming from the builtin.
> > 
> > If you think it would be better to set the target-features in FuncAttrs to 
> > the old value in any case. If that's the case I've added the following code:
> > 
> > if (EnabledForTarget != EnabledForFunc) {
> > FuncAttr.addAttribute(FFeatures);
> > return;
> > }
> You find an incompatible feature and then discontinue processing any further 
> features by early exiting. I expect this to act like an append to any 
> features already present. The incompatibility is at an individual feature 
> level, not the group 
I see. I changed it to match that behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

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

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 555987.
jmmartinez marked 2 inline comments as done.
jmmartinez added a comment.

- Capitalize comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,49 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
 // RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
 
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s
+
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_not_in_target
+// CHECK-SAME: 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 555986.
jmmartinez marked an inline comment as done.
jmmartinez added a comment.

- Review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,49 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
 // RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
 
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s
+
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_not_in_target
+// CHECK-SAME: () 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2017
+for (StringRef Feature : llvm::split(FFeatures.getValueAsString(), ',')) {
+  bool EnabledForFunc = Feature[0] == '+';
+  StringRef Name = Feature.substr(1);

arsenm wrote:
> Do you need to guard against empty string?
I checked and I saw some tests without it. I'm adding the condition for it.



Comment at: clang/lib/CodeGen/CGCall.cpp:2018
+  bool EnabledForFunc = Feature[0] == '+';
+  StringRef Name = Feature.substr(1);
+  auto TEntry = TFeatures.find(Name);

arsenm wrote:
> consume_front
I used drop_front(1) instead, consume_front would only work with "+" and not 
drop "-".



Comment at: clang/lib/CodeGen/CGCall.cpp:2030-2031
+  bool EnabledForTarget = TEntry->second;
+  if (EnabledForTarget != EnabledForFunc)
+return;
+}

arsenm wrote:
> Early return breaks the other features
I did not understand this remark.

If the features are not compatible, we do not add a "target-features" entry in 
the new "FuncAttrs". Then, the old "target-features" entry is kept in the 
Function coming from the builtin.

If you think it would be better to set the target-features in FuncAttrs to the 
old value in any case. If that's the case I've added the following code:

if (EnabledForTarget != EnabledForFunc) {
FuncAttr.addAttribute(FFeatures);
return;
}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

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

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 555691.
jmmartinez marked 4 inline comments as done.
jmmartinez added a comment.

- Review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,50 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,CPU
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,NOCPU
 
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 555051.
jmmartinez added a comment.

- Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,50 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,CPU
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,NOCPU
 
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// 

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19550e79b50f: [NFC][Clang] Remove redundant function 
definitions (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159256

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1259,26 +1259,6 @@
   llvm::AttributeList , unsigned ,
   bool AttrOnCallSite, bool IsThunk);
 
-  /// Adds attributes to F according to our CodeGenOptions and LangOptions, as
-  /// though we had emitted it ourselves.  We remove any attributes on F that
-  /// conflict with the attributes we add here.
-  ///
-  /// This is useful for adding attrs to bitcode modules that you want to link
-  /// with but don't control, such as CUDA's libdevice.  When linking with such
-  /// a bitcode library, you might want to set e.g. its functions'
-  /// "unsafe-fp-math" attribute to match the attr of the functions you're
-  /// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
-  /// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
-  /// will propagate unsafe-fp-math=false up to every transitive caller of a
-  /// function in the bitcode library!
-  ///
-  /// With the exception of fast-math attrs, this will only make the attributes
-  /// on the function more conservative.  But it's unsafe to call this on a
-  /// function which relies on particular fast-math attributes for correctness.
-  /// It's up to you to ensure that this is safe.
-  void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-bool WillInternalize);
-
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -395,10 +395,25 @@
   bool isExternallyDestructed() const { return IsExternallyDestructed; }
 };
 
-/// Helper to add attributes to \p F according to the CodeGenOptions and
-/// LangOptions without requiring a CodeGenModule to be constructed.
+/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
+/// though we had emitted it ourselves. We remove any attributes on F that
+/// conflict with the attributes we add here.
+///
+/// This is useful for adding attrs to bitcode modules that you want to link
+/// with but don't control, such as CUDA's libdevice.  When linking with such
+/// a bitcode library, you might want to set e.g. its functions'
+/// "unsafe-fp-math" attribute to match the attr of the functions you're
+/// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
+/// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
+/// will propagate unsafe-fp-math=false up to every transitive caller of a
+/// function in the bitcode library!
+///
+/// With the exception of fast-math attrs, this will only make the attributes
+/// on the function more conservative.  But it's unsafe to call this on a
+/// function which relies on particular fast-math attributes for correctness.
+/// It's up to you to ensure that this is safe.
 void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-  const CodeGenOptions CodeGenOpts,
+  const CodeGenOptions ,
   const LangOptions ,
   const TargetOptions ,
   bool WillInternalize);
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2002,10 +2002,7 @@
   }
 }
 
-/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
-/// though we had emitted it ourselves. We remove any attributes on F that
-/// conflict with the attributes we add here.
-static void mergeDefaultFunctionDefinitionAttributes(
+void CodeGen::mergeDefaultFunctionDefinitionAttributes(
 llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
@@ -2065,15 +2062,6 @@
   F.addFnAttrs(FuncAttrs);
 }
 
-void clang::CodeGen::mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
- 

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D159256#4630876 , @jhuber6 wrote:

> In D159256#4630410 , @jmmartinez 
> wrote:
>
>> @jhuber6 I was wondering if there is a reason you kept 3 versions of 
>> `mergeDefaultFunctionDefinitionAttributes` in 
>> https://reviews.llvm.org/D152391 ?
>
> I believe it's because one was a freestanding function, the other was a 
> member function, and the last was a common implementation.

Would it be ok if I keep only one? It seems that the member function is not 
used (I was not sure if there was some external code using it).

If not, I can also keep just 2 versions (the freestanding function and the 
member function), move the implementation to the freestanding one, and drop the 
static function since it is redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159256

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

[PATCH] D159257: [Clang] Drop functions with incompatible target-features when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
jmmartinez added reviewers: arsenm, Pierre-vh, yaxunl, jhuber6.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added subscribers: cfe-commits, wdng.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159257

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/link-builtin-bitcode.c


Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -4,11 +4,11 @@
 
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc 
%s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s 
--check-prefixes=CHECK,CPU
+// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
 
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc 
%s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s 
--check-prefixes=CHECK,NOCPU
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s
 
 #ifdef BITCODE
 int no_attr(void) { return 42; }
@@ -40,11 +40,8 @@
 // CHECK-LABEL: define internal i32 @attr_not_in_target
 // CHECK-SAME: () #[[ATTR_EXTEND:[0-9]+]] {
 
-// CHECK-LABEL: @attr_incompatible
-// CHECK-SAME: () #[[ATTR_INCOMPATIBLE:[0-9]+]] {
+// CHECK-NOT: @attr_incompatible
 
 // CHECK: attributes #[[ATTR_BAR]] = { noinline nounwind optnone 
"no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="gfx90a" 
"target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64"
 }
 // CHECK: attributes #[[ATTR_COMPATIBLE]] = { convergent noinline nounwind 
optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="gfx90a" 
"target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64"
 }
 // CHECK: attributes #[[ATTR_EXTEND]] = { convergent noinline nounwind optnone 
"no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="gfx90a" 
"target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+gws,+image-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64,+extended-image-insts"
 }
-// CPU: attributes #[[ATTR_INCOMPATIBLE]] = { convergent noinline nounwind 
optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="gfx90a" 
"target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64,-gfx9-insts"
 }
-// NOCPU: attributes #[[ATTR_INCOMPATIBLE]] = { convergent noinline nounwind 
optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" 
"target-cpu"="gfx90a" "target-features"="-gfx9-insts" }
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -267,11 +267,13 @@
   for (auto  : LinkModules) {
 assert(LM.Module && "LinkModule does not actually have a module");
 if (LM.PropagateAttrs)
-  for (Function  : *LM.Module) {
+  for (Function  : llvm::make_early_inc_range(*LM.Module)) {
 // Skip intrinsics. Keep consistent with how intrinsics are created
 // in LLVM IR.
 if (F.isIntrinsic())
   continue;
+if (CodeGen::dropFunctionWithIncompatibleAttributes(F, TargetOpts))
+  continue;
 CodeGen::mergeDefaultFunctionDefinitionAttributes(
 F, CodeGenOpts, LangOpts, TargetOpts, LM.Internalize);
   }
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -395,6 +395,11 @@
   bool isExternallyDestructed() const { return IsExternallyDestructed; }
 };
 
+/// If \p F "target-features" are incompatible with the \p TargetOpts features,
+/// it is correct to drop the function. \return true if \p F is dropped
+bool 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 554934.
jmmartinez added a comment.

- Rollback, drop incompatible functions in a separate commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,50 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,CPU
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s --check-prefixes=CHECK,NOCPU
 
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define 

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

@jhuber6 I was wondering if there is a reason you kept 3 versions of 
`mergeDefaultFunctionDefinitionAttributes` in https://reviews.llvm.org/D152391 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159256

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

[PATCH] D159256: [NFC][Clang] Remove redundant function definitions

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
jmmartinez added a reviewer: jhuber6.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There were 3 definitions of the mergeDefaultFunctionDefinitionAttributes
function: A private implementation, a version exposed in CodeGen, a
version exposed in CodeGenModule.

This patch removes the private and the CodeGenModule versions and keeps
a single definition in CodeGen.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159256

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1259,26 +1259,6 @@
   llvm::AttributeList , unsigned ,
   bool AttrOnCallSite, bool IsThunk);
 
-  /// Adds attributes to F according to our CodeGenOptions and LangOptions, as
-  /// though we had emitted it ourselves.  We remove any attributes on F that
-  /// conflict with the attributes we add here.
-  ///
-  /// This is useful for adding attrs to bitcode modules that you want to link
-  /// with but don't control, such as CUDA's libdevice.  When linking with such
-  /// a bitcode library, you might want to set e.g. its functions'
-  /// "unsafe-fp-math" attribute to match the attr of the functions you're
-  /// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
-  /// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
-  /// will propagate unsafe-fp-math=false up to every transitive caller of a
-  /// function in the bitcode library!
-  ///
-  /// With the exception of fast-math attrs, this will only make the attributes
-  /// on the function more conservative.  But it's unsafe to call this on a
-  /// function which relies on particular fast-math attributes for correctness.
-  /// It's up to you to ensure that this is safe.
-  void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-bool WillInternalize);
-
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.h
===
--- clang/lib/CodeGen/CGCall.h
+++ clang/lib/CodeGen/CGCall.h
@@ -395,10 +395,25 @@
   bool isExternallyDestructed() const { return IsExternallyDestructed; }
 };
 
-/// Helper to add attributes to \p F according to the CodeGenOptions and
-/// LangOptions without requiring a CodeGenModule to be constructed.
+/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
+/// though we had emitted it ourselves. We remove any attributes on F that
+/// conflict with the attributes we add here.
+///
+/// This is useful for adding attrs to bitcode modules that you want to link
+/// with but don't control, such as CUDA's libdevice.  When linking with such
+/// a bitcode library, you might want to set e.g. its functions'
+/// "unsafe-fp-math" attribute to match the attr of the functions you're
+/// codegen'ing.  Otherwise, LLVM will interpret the bitcode module's lack of
+/// unsafe-fp-math attrs as tantamount to unsafe-fp-math=false, and then LLVM
+/// will propagate unsafe-fp-math=false up to every transitive caller of a
+/// function in the bitcode library!
+///
+/// With the exception of fast-math attrs, this will only make the attributes
+/// on the function more conservative.  But it's unsafe to call this on a
+/// function which relies on particular fast-math attributes for correctness.
+/// It's up to you to ensure that this is safe.
 void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
-  const CodeGenOptions CodeGenOpts,
+  const CodeGenOptions ,
   const LangOptions ,
   const TargetOptions ,
   bool WillInternalize);
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2002,10 +2002,7 @@
   }
 }
 
-/// Adds attributes to \p F according to our \p CodeGenOpts and \p LangOpts, as
-/// though we had emitted it ourselves. We remove any attributes on F that
-/// conflict with the attributes we add here.
-static void mergeDefaultFunctionDefinitionAttributes(
+void CodeGen::mergeDefaultFunctionDefinitionAttributes(
 llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:282
   }
+}
 

Remove { }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

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

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 554753.
jmmartinez marked an inline comment as done.
jmmartinez added a comment.

- Drop incoming builtins with incompatible target-features


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,47 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
 // RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
 
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s
+
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_incompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_incompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_incompatible() + x; }
+
 // CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
+
 // CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
+// CHECK-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
-//
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+atomic-buffer-global-pk-add-f16-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot1-insts,+dot10-insts,+dot2-insts,+dot3-insts,+dot4-insts,+dot5-insts,+dot6-insts,+dot7-insts,+dpp,+gfx8-insts,+gfx9-insts,+gfx90a-insts,+mai-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-// CHECK: attributes #1 = { convergent noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" "target-features"="+16-bit-insts,+ci-insts,+dpp,+gfx8-insts,+s-memrealtime,+s-memtime-inst,+wavefrontsize64" }
-//.
+// CHECK-LABEL: define internal i32 @no_attr
+// CHECK-SAME: () #[[ATTR_COMPATIBLE:[0-9]+]] {
+
+// CHECK-LABEL: define internal i32 @attr_in_target
+// CHECK-SAME: () 

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez marked 2 inline comments as done.
jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2034
+  }
+
+  FuncAttr.addAttribute("target-features", llvm::join(MergedFeatures, ","));

arsenm wrote:
> Really it would be less bad if the incompatible functions were not imported 
> rather than the backend pass
I thought it was possible to have functions with incompatible features in the 
same module. 
e.g. one function compiled with some instruction set support, one without, and 
an ifunc that resolves to one or the other.

Maybe it's not the case in the context of `-mlink-builtin-bitcode`?



Comment at: clang/test/CodeGen/link-builtin-bitcode.c:17
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { 
return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_uncompatible(void) { return 
42; }
 int x = 12;

arsenm wrote:
> This isn't a real target feature (do we not have a warning on this?)
> 
> s/uncompatible/incompatible
Only when the feature doesn't map to any "+" or "-" feature.

If I use "foo-insts" I get a warning because there are no +foo-insts / 
-foo-insts (and it still ends up in the generated llvm-ir)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159206

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

[PATCH] D159206: [Clang] Propagate target-features if compatible when using mlink-builtin-bitcode

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
jmmartinez added reviewers: arsenm, Pierre-vh, yaxunl, jhuber6.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added subscribers: cfe-commits, wdng.
Herald added a project: clang.

Buitlins from AMD's device-libs are compiled without specifying a
target-cpu, which results in builtins without the target-features
attribute set.

Before this patch, when linking this builtins with -mlink-builtin-bitcode
the target-features were not propagated in the incoming builtins.

With this patch, the default target features are propagated
if they are compatible with the target-features in the incoming builtin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159206

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/link-builtin-bitcode.c
  clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu

Index: clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
===
--- clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
+++ clang/test/CodeGenCUDA/link-builtin-bitcode-gpu-attrs-preserved.cu
@@ -31,7 +31,7 @@
 
 
 // CHECK: define {{.*}} i64 @do_intrin_stuff() #[[ATTR:[0-9]+]]
-// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="+gfx11-insts"
+// INTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-cpu"="gfx{{.*}}" "target-features"="{{.*}}+gfx11-insts{{.*}}"
 // NOINTERNALIZE: attributes #[[ATTR]] = {{.*}} "target-features"="+gfx11-insts"
 
 #define __device__ __attribute__((device))
Index: clang/test/CodeGen/link-builtin-bitcode.c
===
--- clang/test/CodeGen/link-builtin-bitcode.c
+++ clang/test/CodeGen/link-builtin-bitcode.c
@@ -1,42 +1,50 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-attributes --check-globals --include-generated-funcs --version 2
+// Build two version of the bitcode library, one with a target-cpu set and one without
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx803 -DBITCODE -emit-llvm-bc -o %t-lib.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -DBITCODE -emit-llvm-bc -o %t-lib.no-cpu.bc %s
+
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
+// RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
+// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s --check-prefixes=COMMON,CPU
+
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple amdgcn-- -target-cpu gfx90a -emit-llvm \
-// RUN:   -mlink-builtin-bitcode %t-lib.bc -o - %t.bc | FileCheck %s
+// RUN:   -mlink-builtin-bitcode %t-lib.no-cpu.bc -o - %t.bc | FileCheck %s --check-prefixes=COMMON,NOCPU
 
 #ifdef BITCODE
-int foo(void) { return 42; }
+int no_attr(void) { return 42; }
+int __attribute__((target("gfx8-insts"))) attr_in_target(void) { return 42; }
+int __attribute__((target("extended-image-insts"))) attr_not_in_target(void) { return 42; }
+int __attribute__((target("no-gfx9-insts"))) attr_uncompatible(void) { return 42; }
 int x = 12;
 #endif
 
-extern int foo(void);
+extern int no_attr(void);
+extern int attr_in_target(void);
+extern int attr_not_in_target(void);
+extern int attr_uncompatible(void);
 extern int x;
 
-int bar() { return foo() + x; }
-//.
-// CHECK: @x = internal addrspace(1) global i32 12, align 4
-//.
-// CHECK: Function Attrs: noinline nounwind optnone
-// CHECK-LABEL: define dso_local i32 @bar
-// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:[[CALL:%.*]] = call i32 @foo()
-// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr addrspacecast (ptr addrspace(1) @x to ptr), align 4
-// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[CALL]], [[TMP0]]
-// CHECK-NEXT:ret i32 [[ADD]]
-//
-//
-// CHECK: Function Attrs: convergent noinline nounwind optnone
-// CHECK-LABEL: define internal i32 @foo
-// CHECK-SAME: () #[[ATTR1:[0-9]+]] {
-// CHECK-NEXT:  entry:
-// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4, addrspace(5)
-// CHECK-NEXT:[[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr
-// CHECK-NEXT:ret i32 42
+int bar() { return no_attr() + attr_in_target() + attr_not_in_target() + attr_uncompatible() + x; }
+
+// COMMON: @x = internal addrspace(1) global i32 12, align 4
+
+// COMMON-LABEL: define dso_local i32 @bar
+// COMMON-SAME: () #[[ATTR_BAR:[0-9]+]] {
 //
-//.
-// CHECK: attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx90a" 

[PATCH] D158990: [NFC][Clang] Remove unused function `CodeGenModule::addDefaultFunctionDefinitionAttributes`

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b352540184e: [NFC][Clang] Remove unused function 
`CodeGenModule… (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158990

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h


Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1276,7 +1276,6 @@
   /// on the function more conservative.  But it's unsafe to call this on a
   /// function which relies on particular fast-math attributes for correctness.
   /// It's up to you to ensure that this is safe.
-  void addDefaultFunctionDefinitionAttributes(llvm::Function );
   void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
 bool WillInternalize);
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2094,14 +2094,6 @@
 addMergableDefaultFunctionAttributes(CodeGenOpts, FuncAttrs);
 }
 
-void CodeGenModule::addDefaultFunctionDefinitionAttributes(llvm::Function ) {
-  llvm::AttrBuilder FuncAttrs(F.getContext());
-  getDefaultFunctionAttributes(F.getName(), F.hasOptNone(),
-   /* AttrOnCallSite = */ false, FuncAttrs);
-  // TODO: call GetCPUAndFeaturesAttributes?
-  F.addFnAttrs(FuncAttrs);
-}
-
 /// Apply default attributes to \p F, accounting for merge semantics of
 /// attributes that should not overwrite existing attributes.
 void CodeGenModule::mergeDefaultFunctionDefinitionAttributes(


Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1276,7 +1276,6 @@
   /// on the function more conservative.  But it's unsafe to call this on a
   /// function which relies on particular fast-math attributes for correctness.
   /// It's up to you to ensure that this is safe.
-  void addDefaultFunctionDefinitionAttributes(llvm::Function );
   void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
 bool WillInternalize);
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2094,14 +2094,6 @@
 addMergableDefaultFunctionAttributes(CodeGenOpts, FuncAttrs);
 }
 
-void CodeGenModule::addDefaultFunctionDefinitionAttributes(llvm::Function ) {
-  llvm::AttrBuilder FuncAttrs(F.getContext());
-  getDefaultFunctionAttributes(F.getName(), F.hasOptNone(),
-   /* AttrOnCallSite = */ false, FuncAttrs);
-  // TODO: call GetCPUAndFeaturesAttributes?
-  F.addFnAttrs(FuncAttrs);
-}
-
 /// Apply default attributes to \p F, accounting for merge semantics of
 /// attributes that should not overwrite existing attributes.
 void CodeGenModule::mergeDefaultFunctionDefinitionAttributes(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158991: [NFC][Clang] Add missing & to function argument

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb63c6e585d86: [NFC][Clang] Add missing  to function 
argument (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158991

Files:
  clang/lib/CodeGen/CGCall.cpp


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2006,7 +2006,7 @@
 /// though we had emitted it ourselves. We remove any attributes on F that
 /// conflict with the attributes we add here.
 static void mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
+llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
 


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2006,7 +2006,7 @@
 /// though we had emitted it ourselves. We remove any attributes on F that
 /// conflict with the attributes we add here.
 static void mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
+llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158991: [NFC][Clang] Add missing & to function argument

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158991

Files:
  clang/lib/CodeGen/CGCall.cpp


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2006,7 +2006,7 @@
 /// though we had emitted it ourselves. We remove any attributes on F that
 /// conflict with the attributes we add here.
 static void mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
+llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
 


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2006,7 +2006,7 @@
 /// though we had emitted it ourselves. We remove any attributes on F that
 /// conflict with the attributes we add here.
 static void mergeDefaultFunctionDefinitionAttributes(
-llvm::Function , const CodeGenOptions CodeGenOpts,
+llvm::Function , const CodeGenOptions ,
 const LangOptions , const TargetOptions ,
 bool WillInternalize) {
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158990: [NFC][Clang] Remove unused function `CodeGenModule::addDefaultFunctionDefinitionAttributes`

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158990

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h


Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1276,7 +1276,6 @@
   /// on the function more conservative.  But it's unsafe to call this on a
   /// function which relies on particular fast-math attributes for correctness.
   /// It's up to you to ensure that this is safe.
-  void addDefaultFunctionDefinitionAttributes(llvm::Function );
   void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
 bool WillInternalize);
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2094,14 +2094,6 @@
 addMergableDefaultFunctionAttributes(CodeGenOpts, FuncAttrs);
 }
 
-void CodeGenModule::addDefaultFunctionDefinitionAttributes(llvm::Function ) {
-  llvm::AttrBuilder FuncAttrs(F.getContext());
-  getDefaultFunctionAttributes(F.getName(), F.hasOptNone(),
-   /* AttrOnCallSite = */ false, FuncAttrs);
-  // TODO: call GetCPUAndFeaturesAttributes?
-  F.addFnAttrs(FuncAttrs);
-}
-
 /// Apply default attributes to \p F, accounting for merge semantics of
 /// attributes that should not overwrite existing attributes.
 void CodeGenModule::mergeDefaultFunctionDefinitionAttributes(


Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1276,7 +1276,6 @@
   /// on the function more conservative.  But it's unsafe to call this on a
   /// function which relies on particular fast-math attributes for correctness.
   /// It's up to you to ensure that this is safe.
-  void addDefaultFunctionDefinitionAttributes(llvm::Function );
   void mergeDefaultFunctionDefinitionAttributes(llvm::Function ,
 bool WillInternalize);
 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2094,14 +2094,6 @@
 addMergableDefaultFunctionAttributes(CodeGenOpts, FuncAttrs);
 }
 
-void CodeGenModule::addDefaultFunctionDefinitionAttributes(llvm::Function ) {
-  llvm::AttrBuilder FuncAttrs(F.getContext());
-  getDefaultFunctionAttributes(F.getName(), F.hasOptNone(),
-   /* AttrOnCallSite = */ false, FuncAttrs);
-  // TODO: call GetCPUAndFeaturesAttributes?
-  F.addFnAttrs(FuncAttrs);
-}
-
 /// Apply default attributes to \p F, accounting for merge semantics of
 /// attributes that should not overwrite existing attributes.
 void CodeGenModule::mergeDefaultFunctionDefinitionAttributes(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149986: AMDGPU: Force sc0 and sc1 on stores for gfx940 and gfx941

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp:524
+ SIAtomicAddrSpace::NONE)
+  return enableSC0Bit(MI) | enableSC1Bit(MI);
+return false;

NIT: Is the use of the bitwise or " | " intended? I'd use the logical or " || " 
instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149986

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D144870#4225437 , @probinson wrote:

> One entirely optional suggestion on the test. LGTM.

Thanks! I used `SECONDDUP` and `FIRSTDUP`.

And thanks a lot for the reviews!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG488185cca387: [Clang][DebugInfo][AMDGPU] Emit zero size 
bitfields in the debug info to… (authored by jmmartinez).

Changed prior to commit:
  https://reviews.llvm.org/D144870?vs=508538=508926#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,119 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes NOSEPARATOR,BOTH %s
+// RUN: %clang_cc1 -triple amdgcn-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes SEPARATOR,BOTH %s
+
+struct First {
+  // BOTH-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // BOTH-DAG: ![[FIRSTDUP:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTDUP_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRSTDUP_ELEMENTS]] = !{![[FIRSTDUP_X:[0-9]+]], ![[FIRSTDUP_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRSTDUP_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTDUP]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRSTDUP_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTDUP]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // BOTH-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // BOTH-DAG: ![[SECONDDUP:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDDUP_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECONDDUP_ELEMENTS]] = !{![[SECONDDUP_X:[0-9]+]], ![[SECONDDUP_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECONDDUP_ELEMENTS]] = !{![[SECONDDUP_X:[0-9]+]], ![[SECONDDUP_ZERO:[0-9]+]], ![[SECONDDUP_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECONDDUP_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDDUP]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECONDDUP_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECONDDUP]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECONDDUP_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECONDDUP]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int : 0;
+  int y : 4;
+};
+
+struct Last {
+  // BOTH-DAG: 

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1563
+
+  assert(PreviousBitfield->isBitField());
+

probinson wrote:
> Is this true for cases like
> ```
> struct nonadjacent {
>   char a : 8;
>   char : 0;
>   int b;
>   char d : 8;
> };
> ```
> where the field `d` has a predecessor that is not a bitfield? (This might be 
> my ignorance of how Decls are put together, but asserting that `advance` is 
> guaranteed to get you a bitfield just seems a little odd.)
In that case the assert is never reached.

When emiting the debug-info for `d`, when looking at the metadata generated for 
the previous field the function should exit early on this condition:
```
  if (!PreviousMDField || !PreviousMDField->isBitField() ||
  PreviousMDField->getSizeInBits() == 0)
return nullptr;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 508538.
jmmartinez marked 3 inline comments as done.
jmmartinez added a comment.

- Took remarks into account


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,119 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes NOSEPARATOR,BOTH %s
+// RUN: %clang_cc1 -triple amdgcn-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes SEPARATOR,BOTH %s
+
+struct First {
+  // BOTH-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // BOTH-DAG: ![[FIRSTD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTD_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRSTD_ELEMENTS]] = !{![[FIRSTD_X:[0-9]+]], ![[FIRSTD_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRSTD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRSTD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // BOTH-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // BOTH-DAG: ![[SECONDD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDD_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_ZERO:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECONDD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECONDD_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECONDD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int : 0;
+  int y : 4;
+};
+
+struct Last {
+  // BOTH-DAG: ![[LAST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Last", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[LAST_ELEMENTS:[0-9]+]])
+  // 

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 506611.
jmmartinez added a comment.

- Updated to use `elements` array to avoid the complicated loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,119 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes NOSEPARATOR,BOTH %s
+// RUN: %clang_cc1 -triple amdgcn-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes SEPARATOR,BOTH %s
+
+struct First {
+  // BOTH-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // BOTH-DAG: ![[FIRSTD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTD_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRSTD_ELEMENTS]] = !{![[FIRSTD_X:[0-9]+]], ![[FIRSTD_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRSTD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRSTD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // BOTH-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // BOTH-DAG: ![[SECONDD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDD_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_ZERO:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECONDD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECONDD_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECONDD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int : 0;
+  int y : 4;
+};
+
+struct Last {
+  // BOTH-DAG: ![[LAST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Last", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[LAST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: 

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D144870#4202121 , @probinson wrote:

> (Of course it's possible that finding the preceding field can be done only by 
> iterating, but I'd hope that isn't necessary.)

Sadly, this is the case. `field_iterator` and the underlying `decl_iterator` 
used to implement it are forward iterators.

However, your comment made me realize that we could forward the `elements` 
array from `CGDebugInfoCollectRecordNormalFields`  which contains the already 
created debug-info for the previous fields. By using this array I could get 
pretty much the same effect. What do you think?

In D144870#4202121 , @probinson wrote:

>   struct non_adjacent_bitfields {
> char d;
> char : 0;
> char e;
> char f : 8;
>   };
>
> (1) Is `e` affected by the presence of the zero-length bitfield? (2) is `f` 
> affected? (3) If the answers are "no" and "yes" then you do need to iterate 
> looking for the zero-length bitfield, but otherwise I think it's sufficient 
> to look only at the preceding field.

(1) and (2): They do not get affected by the precense of the annonymous 
bitfield. Only sequences of bitfields get "squashed" in the same integer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

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

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (5/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez accepted this revision.
jmmartinez added inline comments.
This revision is now accepted and ready to land.



Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:712-714
+  assert(!getAbstractScopeDIEs().count(DS) &&
+ "Abstract DIE for this scope exists!");
+  getAbstractScopeDIEs()[DS] = ScopeDIE;

krisb wrote:
> jmmartinez wrote:
> > NIT: You could use `insert/try_emplace` instead of using `count` and 
> > `operator[]`. The assertion would become something like:
> > ```
> > auto Inserted = getAbstractScopeDIEs().try_emplace(DS, ScopeDIE);
> > assert(Inserted.second && "Abstract DIE for this scope exists!");
> > return ScopeDIE;
> > ```
> I'd slightly prefer to keep the original code as it looks a bit more readable 
> to me (in your example, there should be another line to void out unused 
> `Inserted` variable, otherwise it'd cause a warning). 
> Additional `count()` call in the `assert()` doesn't seem too expensive to me, 
> but if you still think `try_emplace()` is better, I'll change to it.
Ok for me. If the use of count was outside the assert I would have argued 
against.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1558
+  EmitSeparator = FieldIt->isBitField();
+  }
+

probinson wrote:
> I might not be following this correctly, but it feels like EmitSeparator will 
> end up true if the last field is a bitfield, even if there are no zero-length 
> bitfields in front of it. The test does not cover this case (to show that the 
> no-zero-bitfields case is handled properly).
Ouch! I feel ashamed about that loop I originally wrote.

I've added a case covering the non-zero-bitfields case and fixed the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 505507.
jmmartinez added a comment.

- Fixed the loop and added test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,119 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes NOSEPARATOR,BOTH %s
+// RUN: %clang_cc1 -triple amdgcn-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes SEPARATOR,BOTH %s
+
+struct First {
+  // BOTH-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // BOTH-DAG: ![[FIRSTD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTD_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRSTD_ELEMENTS]] = !{![[FIRSTD_X:[0-9]+]], ![[FIRSTD_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRSTD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRSTD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // BOTH-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // BOTH-DAG: ![[SECONDD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDD_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_ZERO:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECONDD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECONDD_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECONDD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int : 0;
+  int y : 4;
+};
+
+struct Last {
+  // BOTH-DAG: ![[LAST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Last", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[LAST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[LAST_ELEMENTS]] = 

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 505148.
jmmartinez added a comment.

Update:

- Took into account remarks and got my ideas straight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/CodeGen/TargetInfo.h
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,106 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes NOSEPARATOR,BOTH %s
+// RUN: %clang_cc1 -triple amdgcn-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck --check-prefixes SEPARATOR,BOTH %s
+
+struct First {
+  // BOTH-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // BOTH-DAG: ![[FIRSTD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTD_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: ![[FIRSTD_ELEMENTS]] = !{![[FIRSTD_X:[0-9]+]], ![[FIRSTD_Y:[0-9]+]]}
+  // BOTH-DAG: ![[FIRSTD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // BOTH-DAG: ![[FIRSTD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // BOTH-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // BOTH-DAG: ![[SECONDD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDD_ELEMENTS:[0-9]+]])
+
+  // NOSEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+  // SEPARATOR-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_ZERO:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+
+  // BOTH-DAG: ![[SECONDD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // SEPARATOR-DAG: ![[SECONDD_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // BOTH-DAG: ![[SECONDD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int : 0;
+  int y : 4;
+};
+
+struct Last {
+  // BOTH-DAG: ![[LAST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Last", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[LAST_ELEMENTS:[0-9]+]])
+  // BOTH-DAG: 

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (5/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

Just a few minor comments. Everything else seems good to me.




Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:698
+  // an inlined function: if a local variable has a templated type with
+  // a function-local type as a template parameter. See PR55680 for details.
+  if (!Scope->isAbstractScope() && !Scope->getInlinedAt()) {

I cannot find the link to PR55680. Would you mind sharing it?

You could also reference `local-type-as-template-parameter.ll`, your test 
depicts the issue very clearly.



Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:700-701
+  if (!Scope->isAbstractScope() && !Scope->getInlinedAt()) {
+if (LexicalBlockDIEs.count(DS)) {
+  ScopeDIE = LexicalBlockDIEs[DS];
+  assert(!ScopeDIE->findAttribute(dwarf::DW_AT_low_pc) &&

NIT: You could use `find` to avoid searching in `LexicalBlockDIEs` twice.



Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:712-714
+  assert(!getAbstractScopeDIEs().count(DS) &&
+ "Abstract DIE for this scope exists!");
+  getAbstractScopeDIEs()[DS] = ScopeDIE;

NIT: You could use `insert/try_emplace` instead of using `count` and 
`operator[]`. The assertion would become something like:
```
auto Inserted = getAbstractScopeDIEs().try_emplace(DS, ScopeDIE);
assert(Inserted.second && "Abstract DIE for this scope exists!");
return ScopeDIE;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144006

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

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

Sorry for the delay, I was out-of-office. Patches 1 to 4 look good to me. If 
nobody raises concerns this week I'll mark them as accepted.

I'm currently reviewing patch 5.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143984

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

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
jmmartinez added reviewers: dblaikie, probinson.
Herald added subscribers: kosarev, tpr.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I'm looking for some feedback, this is definitively not the final solution.
I was wondering if there was an alternative to pass this information in the 
dwarf debug-info.

Consider the following sturctures when targetting:

  struct foo {
int space[4];
char a : 8;
char b : 8;
char x : 8;
char y : 8;
  };
  
  struct bar {
int space[4];
char a : 8;
char b : 8;
char : 0;
char x : 8;
char y : 8;
  };

Even if both structs have the same layout in memory, they are handled
differenlty by the AMDGPU ABI.

With the following code:

// clang --target=amdgcn-amd-amdhsa -g -O1 example.c -S
char use_foo(struct foo f) { return f.y; }
char use_bar(struct bar b) { return b.y; }

For use_foo, the 'y' field is passed in v4
; v_ashrrev_i32_e32 v0, 24, v4
; s_setpc_b64 s[30:31]

For use_bar, the 'y' field is passed in v5
; v_bfe_i32 v0, v5, 8, 8
; s_setpc_b64 s[30:31]

To make this distinction, we record a single 0-size bitfield for every member 
that is preceded
by it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144870

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGRecordLayout.h
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/test/CodeGen/debug-info-bitfield-0-struct.c

Index: clang/test/CodeGen/debug-info-bitfield-0-struct.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-bitfield-0-struct.c
@@ -0,0 +1,97 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -o - -emit-llvm -debug-info-kind=limited %s | FileCheck %s
+
+struct First {
+  // CHECK-DAG: ![[FIRST:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "First", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRST_ELEMENTS:[0-9]+]])
+  // CHECK-DAG: ![[FIRST_ELEMENTS]] = !{![[FIRST_X:[0-9]+]], ![[FIRST_Y:[0-9]+]]}
+  // CHECK-DAG: ![[FIRST_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // CHECK-DAG: ![[FIRST_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRST]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct FirstDuplicate {
+  // CHECK-DAG: ![[FIRSTD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "FirstDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 32, elements: ![[FIRSTD_ELEMENTS:[0-9]+]])
+  // CHECK-DAG: ![[FIRSTD_ELEMENTS]] = !{![[FIRSTD_X:[0-9]+]], ![[FIRSTD_Y:[0-9]+]]}
+  // CHECK-DAG: ![[FIRSTD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // CHECK-DAG: ![[FIRSTD_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[FIRSTD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 4, flags: DIFlagBitField, extraData: i64 0)
+  int : 0;
+  int : 0;
+  int x : 4;
+  int y : 4;
+};
+
+struct Second {
+  // CHECK-DAG: ![[SECOND:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Second", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECOND_ELEMENTS:[0-9]+]])
+  // CHECK-DAG: ![[SECOND_ELEMENTS]] = !{![[SECOND_X:[0-9]+]], ![[SECOND_ZERO:[0-9]+]], ![[SECOND_Y:[0-9]+]]}
+  // CHECK-DAG: ![[SECOND_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: i64 0)
+  // CHECK-DAG: ![[SECOND_ZERO]] = !DIDerivedType(tag: DW_TAG_member, scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  // CHECK-DAG: ![[SECOND_Y]] = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: ![[SECOND]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, offset: 32, flags: DIFlagBitField, extraData: i64 32)
+  int x : 4;
+  int : 0;
+  int y : 4;
+};
+
+struct SecondDuplicate {
+  // CHECK-DAG: ![[SECONDD:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "SecondDuplicate", file: !{{[0-9]+}}, line: {{[0-9]+}}, size: 64, elements: ![[SECONDD_ELEMENTS:[0-9]+]])
+  // CHECK-DAG: ![[SECONDD_ELEMENTS]] = !{![[SECONDD_X:[0-9]+]], ![[SECONDD_ZERO:[0-9]+]], ![[SECONDD_Y:[0-9]+]]}
+  // CHECK-DAG: ![[SECONDD_X]] = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: ![[SECONDD]], file: !{{[0-9]+}}, line: {{[0-9]+}}, baseType: !{{[0-9]+}}, size: 4, flags: DIFlagBitField, extraData: 

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: llvm/lib/IR/DIBuilder.cpp:789
+  return createLocalVariable(
+  VMContext, getSubprogramNodesTrackingVector(Scope), Scope, Name,
+  /* ArgNo */ 0, File, LineNo, Ty, AlwaysPreserve, Flags, AlignInBits);

krisb wrote:
> jmmartinez wrote:
> > I'll be tempted to move the call to 
> > `getSubprogramNodesTrackingVector(Scope)` into `createLocalVariable`. It 
> > seems that every time `createLocalVariable` is called, 
> > `getSubprogramNodesTrackingVector(Scope)` is passed as value for 
> > `PreservedNodes`.
> > 
> > (I've just started reviewing so I may be missing some other modifications)
> That's right, but the problem is in the fact that `createLocalVariable()` is 
> static while `getSubprogramNodesTrackingVector()` is a DIBuilder's member. To 
> move the call inside `createLocalVariable()` we need either to make it a 
> member of DIBuilder or to pass DIBuilder object to it. None of these options 
> looks much better to me, honestly. 
You're right. I didn't realize createLocalVariable was not a member of 
DIBuilder.

Ok for me to keep it as it .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143984

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

[PATCH] D144004: [DebugMetadata][DwarfDebug] Fix DWARF emisson of function-local imported entities (3/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

I've added just a few minor remarks.




Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1082-1084
+  if (!includeMinimalInlineScopes() && !Scope->getInlinedAt())
+for (const auto *Decl : DD->getLocalDeclsForScope(Scope->getScopeNode()))
+  DeferredLocalDecls.insert(Decl);

NIT: You could avoid writing the for loop



Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1646-1649
+  if (LexicalBlockDIEs.count(LB))
+return LexicalBlockDIEs[LB];
+
+  return nullptr;

Nit: `lookup` returns the value in the map or returns a default value.



Comment at: llvm/test/DebugInfo/Generic/split-dwarf-local-import3.ll:17-36
+; CHECK:   DW_TAG_subprogram
+; CHECK: DW_AT_name("foo")
+; CHECK: DW_TAG_imported_declaration
+; CHECK: NULL
+
+; CHECK:   DW_TAG_base_type
+; CHECK: DW_AT_name("int")

I'd be tempted to match the offset of the abstract subprogram and of the 
imported declaration too.
At least for me, it makes clear the intention of the test without running it.

What do you think ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144004

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

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: llvm/lib/IR/DIBuilder.cpp:789
+  return createLocalVariable(
+  VMContext, getSubprogramNodesTrackingVector(Scope), Scope, Name,
+  /* ArgNo */ 0, File, LineNo, Ty, AlwaysPreserve, Flags, AlignInBits);

I'll be tempted to move the call to `getSubprogramNodesTrackingVector(Scope)` 
into `createLocalVariable`. It seems that every time `createLocalVariable` is 
called, `getSubprogramNodesTrackingVector(Scope)` is passed as value for 
`PreservedNodes`.

(I've just started reviewing so I may be missing some other modifications)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143984

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

[PATCH] D141700: AMDGPU: Move enqueued block handling into clang

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:12581
+  Mod, HandleTy,
+  /*isConstant=*/true, llvm::GlobalValue::InternalLinkage,
+  /*Initializer=*/RuntimeHandleInitializer, RuntimeHandleName,

Just a cosmetical remark: Is there any reason to keep the `/*isConstant=*/`, 
`/*Initializer=*/`, ... comments? I think it would be better to avoid them.


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

https://reviews.llvm.org/D141700

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

[PATCH] D140195: [Clang][CGDebugInfo][ObjC] Mark objc bitfields with the DIFlagBitfield flag

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1118ee04fc7c: [Clang][CGDebugInfo][ObjC] Mark objc bitfields 
with the DIFlagBitfield flag (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140195

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-ivars.m


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2930,6 +2930,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if (Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2930,6 +2930,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if (Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140195: [Clang][CGDebugInfo][ObjC] Mark objc bitfields with the DIFlagBitfield flag

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D140195#4008724 , @aprantl wrote:

>> Unluckly, I wasn't able to test the combination of 
>> https://reviews.llvm.org/D96334 and this patch together. If you have some 
>> pointers about how to trigger a test job on "Green Dragon", or how to test 
>> lldb's objective-c tests on Linux I'd be happy to test.
>
> There's no way to request a test from green dragon. There is also no way to 
> test LLDB's Objective-C support on Linux, or at least I'm not aware of anyone 
> maintaining a port of LLDB to the GNUstep Objective-C runtime.
> So I tried out the patch on a Mac locally and the entire LLDB testsuite still 
> passes.

Thanks for the help in testing! Did you happen to test with 
https://reviews.llvm.org/D96334 ? If you confirm I'll "revert the revert" 
(commit 920de9c94caff0b3ac21bf637487b07cb9aea98a 
).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140195

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

[PATCH] D140195: [Clang][CGDebugInfo][ObjC] Mark objc bitfields with the DIFlagBitfield flag

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

In D140195#4001989 , @aprantl wrote:

> How does this affect the generated DWARF? Have you tested that this does the 
> right thing in LLDB?

It shouldn't affect the generated DWARF on LLVM's main branch.
Currently, LLVM relies on the field size to know if a member is a bitfield or 
not.

However, if we apply the patch https://reviews.llvm.org/D96334 , if the 
`DIFlagBitField` attribute is not added, the members won't be treated as 
bitfields.

Unluckly, I wasn't able to test the combination of 
https://reviews.llvm.org/D96334 and this patch together. If you have some 
pointers about how to trigger a test job on "Green Dragon", or how to test 
lldb's objective-c tests on Linux I'd be happy to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140195

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

[PATCH] D140195: [Clang][CGDebugInfo][ObjC] Mark objc bitfields with the DIFlagBitfield flag

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez updated this revision to Diff 483468.
jmmartinez added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140195

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-ivars.m


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2942,6 +2942,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if (Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2942,6 +2942,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if (Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140195: [Clang][CGDebugInfo][ObjC] Mark objc bitfields with the DIFlagBitfield flag

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140195

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenObjC/debug-info-ivars.m


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2942,6 +2942,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if(Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =


Index: clang/test/CodeGenObjC/debug-info-ivars.m
===
--- clang/test/CodeGenObjC/debug-info-ivars.m
+++ clang/test/CodeGenObjC/debug-info-ivars.m
@@ -30,15 +30,15 @@
 // CHECK-SAME:   baseType: ![[UNSIGNED:[0-9]+]]
 // CHECK-SAME:   size: 9,
 // CHECK-NOT:offset:
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: ![[UNSIGNED]] = !DIBasicType(name: "unsigned int"
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_2"
 // CHECK-SAME:   line: 12
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 1,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
 // CHECK: !DIDerivedType(tag: DW_TAG_member, name: "flag_3"
 // CHECK-SAME:   line: 14
 // CHECK-SAME:   baseType: ![[UNSIGNED]]
 // CHECK-SAME:   size: 9, offset: 3,
-// CHECK-SAME:   flags: DIFlagProtected
+// CHECK-SAME:   flags: DIFlagProtected | DIFlagBitField
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2942,6 +2942,9 @@
 else if (Field->getAccessControl() == ObjCIvarDecl::Public)
   Flags = llvm::DINode::FlagPublic;
 
+if(Field->isBitField())
+  Flags |= llvm::DINode::FlagBitField;
+
 llvm::MDNode *PropertyNode = nullptr;
 if (ObjCImplementationDecl *ImpD = ID->getImplementation()) {
   if (ObjCPropertyImplDecl *PImpD =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139023: [NFC][Clang][Driver][AMDGPU] Avoid temporary copies of std::string by using Twine and StringRef

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez added a comment.

@tra Thanks for the review! I double checked with ASAN and there were no issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139023

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

[PATCH] D139023: [NFC][Clang][Driver][AMDGPU] Avoid temporary copies of std::string by using Twine and StringRef

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa446827249bd: [NFC][Clang][Driver][AMDGPU] Avoid temporary 
copies of std::string by using… (authored by jmmartinez).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139023

Files:
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/HIPAMD.cpp
  clang/lib/Driver/ToolChains/ROCm.h

Index: clang/lib/Driver/ToolChains/ROCm.h
===
--- clang/lib/Driver/ToolChains/ROCm.h
+++ clang/lib/Driver/ToolChains/ROCm.h
@@ -252,8 +252,11 @@
   }
 
   /// Get libdevice file for given architecture
-  std::string getLibDeviceFile(StringRef Gpu) const {
-return LibDeviceMap.lookup(Gpu);
+  StringRef getLibDeviceFile(StringRef Gpu) const {
+auto Loc = LibDeviceMap.find(Gpu);
+if (Loc == LibDeviceMap.end())
+  return "";
+return Loc->second;
   }
 
   void AddHIPIncludeArgs(const llvm::opt::ArgList ,
@@ -263,7 +266,7 @@
   void detectHIPRuntime();
 
   /// Get the values for --rocm-device-lib-path arguments
-  std::vector getRocmDeviceLibPathArg() const {
+  const ArrayRef getRocmDeviceLibPathArg() const {
 return RocmDeviceLibPathArg;
   }
 
@@ -273,7 +276,7 @@
   /// Get the value for --hip-version argument
   StringRef getHIPVersionArg() const { return HIPVersionArg; }
 
-  std::string getHIPVersion() const { return DetectedVersion; }
+  StringRef getHIPVersion() const { return DetectedVersion; }
 };
 
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/HIPAMD.cpp
===
--- clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -237,7 +237,7 @@
   DriverArgs.getLastArgValue(options::OPT_gpu_max_threads_per_block_EQ);
   if (!MaxThreadsPerBlock.empty()) {
 std::string ArgStr =
-std::string("--gpu-max-threads-per-block=") + MaxThreadsPerBlock.str();
+(Twine("--gpu-max-threads-per-block=") + MaxThreadsPerBlock).str();
 CC1Args.push_back(DriverArgs.MakeArgStringRef(ArgStr));
   }
 
@@ -344,7 +344,7 @@
   ArgStringList LibraryPaths;
 
   // Find in --hip-device-lib-path and HIP_LIBRARY_PATH.
-  for (auto Path : RocmInstallation.getRocmDeviceLibPathArg())
+  for (StringRef Path : RocmInstallation.getRocmDeviceLibPathArg())
 LibraryPaths.push_back(DriverArgs.MakeArgString(Path));
 
   addDirectoryList(DriverArgs, LibraryPaths, "", "HIP_DEVICE_LIB_PATH");
@@ -354,7 +354,7 @@
   if (!BCLibArgs.empty()) {
 llvm::for_each(BCLibArgs, [&](StringRef BCName) {
   StringRef FullName;
-  for (std::string LibraryPath : LibraryPaths) {
+  for (StringRef LibraryPath : LibraryPaths) {
 SmallString<128> Path(LibraryPath);
 llvm::sys::path::append(Path, BCName);
 FullName = Path;
@@ -387,15 +387,15 @@
 getDriver().Diag(DiagID);
 return {};
   } else
-BCLibs.push_back({AsanRTL.str(), /*ShouldInternalize=*/false});
+BCLibs.emplace_back(AsanRTL, /*ShouldInternalize=*/false);
 }
 
 // Add the HIP specific bitcode library.
 BCLibs.push_back(RocmInstallation.getHIPPath());
 
 // Add common device libraries like ocml etc.
-for (auto N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
-  BCLibs.push_back(StringRef(N));
+for (StringRef N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
+  BCLibs.emplace_back(N);
 
 // Add instrument lib.
 auto InstLib =
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -854,7 +854,7 @@
   const StringRef GpuArch = getGPUArch(DriverArgs);
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
-  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
+  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
   auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
   getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
   if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
@@ -946,7 +946,7 @@
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GPUArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
 
-  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
+  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
   auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
   getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
   if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D139023: [NFC][Clang][Driver][AMDGPU] Avoid temporary copies of std::string by using Twine and StringRef

[Juan Manuel Martinez Caamaño via Phabricator via cfe-commits]

jmmartinez created this revision.
Herald added subscribers: kosarev, kerbowa, tpr, dstuttard, yaxunl, jvesely, 
kzhuravl.
Herald added a project: All.
jmmartinez requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, wdng.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139023

Files:
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/HIPAMD.cpp
  clang/lib/Driver/ToolChains/ROCm.h

Index: clang/lib/Driver/ToolChains/ROCm.h
===
--- clang/lib/Driver/ToolChains/ROCm.h
+++ clang/lib/Driver/ToolChains/ROCm.h
@@ -252,8 +252,11 @@
   }
 
   /// Get libdevice file for given architecture
-  std::string getLibDeviceFile(StringRef Gpu) const {
-return LibDeviceMap.lookup(Gpu);
+  StringRef getLibDeviceFile(StringRef Gpu) const {
+auto Loc = LibDeviceMap.find(Gpu);
+if (Loc == LibDeviceMap.end())
+  return "";
+return Loc->second;
   }
 
   void AddHIPIncludeArgs(const llvm::opt::ArgList ,
@@ -263,7 +266,7 @@
   void detectHIPRuntime();
 
   /// Get the values for --rocm-device-lib-path arguments
-  std::vector getRocmDeviceLibPathArg() const {
+  const ArrayRef getRocmDeviceLibPathArg() const {
 return RocmDeviceLibPathArg;
   }
 
@@ -273,7 +276,7 @@
   /// Get the value for --hip-version argument
   StringRef getHIPVersionArg() const { return HIPVersionArg; }
 
-  std::string getHIPVersion() const { return DetectedVersion; }
+  StringRef getHIPVersion() const { return DetectedVersion; }
 };
 
 } // end namespace driver
Index: clang/lib/Driver/ToolChains/HIPAMD.cpp
===
--- clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -237,7 +237,7 @@
   DriverArgs.getLastArgValue(options::OPT_gpu_max_threads_per_block_EQ);
   if (!MaxThreadsPerBlock.empty()) {
 std::string ArgStr =
-std::string("--gpu-max-threads-per-block=") + MaxThreadsPerBlock.str();
+(Twine("--gpu-max-threads-per-block=") + MaxThreadsPerBlock).str();
 CC1Args.push_back(DriverArgs.MakeArgStringRef(ArgStr));
   }
 
@@ -344,7 +344,7 @@
   ArgStringList LibraryPaths;
 
   // Find in --hip-device-lib-path and HIP_LIBRARY_PATH.
-  for (auto Path : RocmInstallation.getRocmDeviceLibPathArg())
+  for (StringRef Path : RocmInstallation.getRocmDeviceLibPathArg())
 LibraryPaths.push_back(DriverArgs.MakeArgString(Path));
 
   addDirectoryList(DriverArgs, LibraryPaths, "", "HIP_DEVICE_LIB_PATH");
@@ -354,7 +354,7 @@
   if (!BCLibArgs.empty()) {
 llvm::for_each(BCLibArgs, [&](StringRef BCName) {
   StringRef FullName;
-  for (std::string LibraryPath : LibraryPaths) {
+  for (StringRef LibraryPath : LibraryPaths) {
 SmallString<128> Path(LibraryPath);
 llvm::sys::path::append(Path, BCName);
 FullName = Path;
@@ -387,15 +387,15 @@
 getDriver().Diag(DiagID);
 return {};
   } else
-BCLibs.push_back({AsanRTL.str(), /*ShouldInternalize=*/false});
+BCLibs.emplace_back(AsanRTL, /*ShouldInternalize=*/false);
 }
 
 // Add the HIP specific bitcode library.
 BCLibs.push_back(RocmInstallation.getHIPPath());
 
 // Add common device libraries like ocml etc.
-for (auto N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
-  BCLibs.push_back(StringRef(N));
+for (StringRef N : getCommonDeviceLibNames(DriverArgs, GpuArch.str()))
+  BCLibs.emplace_back(N);
 
 // Add instrument lib.
 auto InstLib =
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -854,7 +854,7 @@
   const StringRef GpuArch = getGPUArch(DriverArgs);
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
-  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
+  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
   auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
   getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
   if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
@@ -946,7 +946,7 @@
   auto Kind = llvm::AMDGPU::parseArchAMDGCN(GPUArch);
   const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind);
 
-  std::string LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
+  StringRef LibDeviceFile = RocmInstallation.getLibDeviceFile(CanonArch);
   auto ABIVer = DeviceLibABIVersion::fromCodeObjectVersion(
   getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
   if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org