[clang] [clang codegen] Delete unnecessary GEP cleanup code. (PR #90303)

2024-05-28 Thread Eli Friedman via cfe-commits 

https://github.com/efriedma-quic closed 
https://github.com/llvm/llvm-project/pull/90303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang codegen] Delete unnecessary GEP cleanup code. (PR #90303)

2024-05-28 Thread Richard Smith via cfe-commits 

https://github.com/zygoloid approved this pull request.


https://github.com/llvm/llvm-project/pull/90303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang codegen] Delete unnecessary GEP cleanup code. (PR #90303)

2024-05-28 Thread Eli Friedman via cfe-commits 

https://github.com/efriedma-quic updated 
https://github.com/llvm/llvm-project/pull/90303

>From 4a3612bcf0e6dd3a68e2b648bb662b4faf154b26 Mon Sep 17 00:00:00 2001
From: Eli Friedman 
Date: Fri, 26 Apr 2024 16:58:57 -0700
Subject: [PATCH] [clang codegen] Delete unnecessary GEP cleanup code.

There's some code in AggExprEmitter::VisitCXXParenListOrInitListExpr to
try to do early cleanup for GEPs for fields that aren't accessed. But
it's unlikely to actually save significant compile-time, and it's subtly
wrong in cases where EmitLValueForFieldInitialization() doesn't create a
GEP.  So just delete the code.

Fixes #88077. Fixes #89547.
---
 clang/lib/CodeGen/CGExprAgg.cpp | 10 -
 clang/test/CodeGenCXX/no-unique-address.cpp | 25 +
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 355fec42be448..61db6293ed6a1 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -1811,7 +1811,6 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 // Push a destructor if necessary.
 // FIXME: if we have an array of structures, all explicitly
 // initialized, we can end up pushing a linear number of cleanups.
-bool pushedCleanup = false;
 if (QualType::DestructionKind dtorKind
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
@@ -1819,17 +1818,8 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), field->getType(),
 CGF.getDestroyer(dtorKind), false);
 addCleanup(CGF.EHStack.stable_begin());
-pushedCleanup = true;
   }
 }
-
-// If the GEP didn't get used because of a dead zero init or something
-// else, clean it up for -O0 builds and general tidiness.
-if (!pushedCleanup && LV.isSimple())
-  if (llvm::GetElementPtrInst *GEP =
-  dyn_cast(LV.emitRawPointer(CGF)))
-if (GEP->use_empty())
-  GEP->eraseFromParent();
   }
 
   // Deactivate all the partial cleanups in reverse order, which
diff --git a/clang/test/CodeGenCXX/no-unique-address.cpp 
b/clang/test/CodeGenCXX/no-unique-address.cpp
index 7b4bbbf2a05d5..82532c5e1be82 100644
--- a/clang/test/CodeGenCXX/no-unique-address.cpp
+++ b/clang/test/CodeGenCXX/no-unique-address.cpp
@@ -101,3 +101,28 @@ struct HasZeroSizedFieldWithNonTrivialInit {
 HasZeroSizedFieldWithNonTrivialInit testHasZeroSizedFieldWithNonTrivialInit = 
{.a = 1};
 // CHECK-LABEL: define {{.*}}cxx_global_var_init
 // CHECK: call 
{{.*}}@_ZN14NonTrivialInitC1Ev({{.*}}@testHasZeroSizedFieldWithNonTrivialInit
+
+void *operator new(unsigned long, void *);
+template 
+struct _box {
+  [[no_unique_address]] Ty _value;
+};
+// Make sure this doesn't crash.
+// CHECK-LABEL: define {{.*}}placement_new_struct
+void placement_new_struct() {
+  struct set_value_t {};
+
+  // GH88077
+  struct _tuple : _box, _box {};
+
+  int _storage[1];
+  new (_storage) _tuple{};
+
+  // GH89547
+  struct _tuple2 {
+_box a;
+  };
+
+  int _storage2[1];
+  new (_storage2) _tuple2{};
+}

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

[clang] [clang codegen] Delete unnecessary GEP cleanup code. (PR #90303)

2024-04-26 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-codegen

Author: Eli Friedman (efriedma-quic)


Changes

There's some code in AggExprEmitter::VisitCXXParenListOrInitListExpr to try to 
do early cleanup for GEPs for fields that aren't accessed. But it's unlikely to 
actually save significant compile-time, and it's subtly wrong in cases where 
EmitLValueForFieldInitialization() doesn't create a GEP.  So just delete the 
code.

Fixes #88077. Fixes #89547.

---
Full diff: https://github.com/llvm/llvm-project/pull/90303.diff


2 Files Affected:

- (modified) clang/lib/CodeGen/CGExprAgg.cpp (-10) 
- (modified) clang/test/CodeGenCXX/no-unique-address.cpp (+25) 


``diff
diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 355fec42be4489..61db6293ed6a1e 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -1811,7 +1811,6 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 // Push a destructor if necessary.
 // FIXME: if we have an array of structures, all explicitly
 // initialized, we can end up pushing a linear number of cleanups.
-bool pushedCleanup = false;
 if (QualType::DestructionKind dtorKind
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
@@ -1819,17 +1818,8 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), field->getType(),
 CGF.getDestroyer(dtorKind), false);
 addCleanup(CGF.EHStack.stable_begin());
-pushedCleanup = true;
   }
 }
-
-// If the GEP didn't get used because of a dead zero init or something
-// else, clean it up for -O0 builds and general tidiness.
-if (!pushedCleanup && LV.isSimple())
-  if (llvm::GetElementPtrInst *GEP =
-  dyn_cast(LV.emitRawPointer(CGF)))
-if (GEP->use_empty())
-  GEP->eraseFromParent();
   }
 
   // Deactivate all the partial cleanups in reverse order, which
diff --git a/clang/test/CodeGenCXX/no-unique-address.cpp 
b/clang/test/CodeGenCXX/no-unique-address.cpp
index 7b4bbbf2a05d51..82532c5e1be82a 100644
--- a/clang/test/CodeGenCXX/no-unique-address.cpp
+++ b/clang/test/CodeGenCXX/no-unique-address.cpp
@@ -101,3 +101,28 @@ struct HasZeroSizedFieldWithNonTrivialInit {
 HasZeroSizedFieldWithNonTrivialInit testHasZeroSizedFieldWithNonTrivialInit = 
{.a = 1};
 // CHECK-LABEL: define {{.*}}cxx_global_var_init
 // CHECK: call 
{{.*}}@_ZN14NonTrivialInitC1Ev({{.*}}@testHasZeroSizedFieldWithNonTrivialInit
+
+void *operator new(unsigned long, void *);
+template 
+struct _box {
+  [[no_unique_address]] Ty _value;
+};
+// Make sure this doesn't crash.
+// CHECK-LABEL: define {{.*}}placement_new_struct
+void placement_new_struct() {
+  struct set_value_t {};
+
+  // GH88077
+  struct _tuple : _box, _box {};
+
+  int _storage[1];
+  new (_storage) _tuple{};
+
+  // GH89547
+  struct _tuple2 {
+_box a;
+  };
+
+  int _storage2[1];
+  new (_storage2) _tuple2{};
+}

``




https://github.com/llvm/llvm-project/pull/90303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang codegen] Delete unnecessary GEP cleanup code. (PR #90303)

2024-04-26 Thread Eli Friedman via cfe-commits 

https://github.com/efriedma-quic created 
https://github.com/llvm/llvm-project/pull/90303

There's some code in AggExprEmitter::VisitCXXParenListOrInitListExpr to try to 
do early cleanup for GEPs for fields that aren't accessed. But it's unlikely to 
actually save significant compile-time, and it's subtly wrong in cases where 
EmitLValueForFieldInitialization() doesn't create a GEP.  So just delete the 
code.

Fixes #88077. Fixes #89547.

>From 4a3612bcf0e6dd3a68e2b648bb662b4faf154b26 Mon Sep 17 00:00:00 2001
From: Eli Friedman 
Date: Fri, 26 Apr 2024 16:58:57 -0700
Subject: [PATCH] [clang codegen] Delete unnecessary GEP cleanup code.

There's some code in AggExprEmitter::VisitCXXParenListOrInitListExpr to
try to do early cleanup for GEPs for fields that aren't accessed. But
it's unlikely to actually save significant compile-time, and it's subtly
wrong in cases where EmitLValueForFieldInitialization() doesn't create a
GEP.  So just delete the code.

Fixes #88077. Fixes #89547.
---
 clang/lib/CodeGen/CGExprAgg.cpp | 10 -
 clang/test/CodeGenCXX/no-unique-address.cpp | 25 +
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 355fec42be4489..61db6293ed6a1e 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -1811,7 +1811,6 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 // Push a destructor if necessary.
 // FIXME: if we have an array of structures, all explicitly
 // initialized, we can end up pushing a linear number of cleanups.
-bool pushedCleanup = false;
 if (QualType::DestructionKind dtorKind
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
@@ -1819,17 +1818,8 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
 CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), field->getType(),
 CGF.getDestroyer(dtorKind), false);
 addCleanup(CGF.EHStack.stable_begin());
-pushedCleanup = true;
   }
 }
-
-// If the GEP didn't get used because of a dead zero init or something
-// else, clean it up for -O0 builds and general tidiness.
-if (!pushedCleanup && LV.isSimple())
-  if (llvm::GetElementPtrInst *GEP =
-  dyn_cast(LV.emitRawPointer(CGF)))
-if (GEP->use_empty())
-  GEP->eraseFromParent();
   }
 
   // Deactivate all the partial cleanups in reverse order, which
diff --git a/clang/test/CodeGenCXX/no-unique-address.cpp 
b/clang/test/CodeGenCXX/no-unique-address.cpp
index 7b4bbbf2a05d51..82532c5e1be82a 100644
--- a/clang/test/CodeGenCXX/no-unique-address.cpp
+++ b/clang/test/CodeGenCXX/no-unique-address.cpp
@@ -101,3 +101,28 @@ struct HasZeroSizedFieldWithNonTrivialInit {
 HasZeroSizedFieldWithNonTrivialInit testHasZeroSizedFieldWithNonTrivialInit = 
{.a = 1};
 // CHECK-LABEL: define {{.*}}cxx_global_var_init
 // CHECK: call 
{{.*}}@_ZN14NonTrivialInitC1Ev({{.*}}@testHasZeroSizedFieldWithNonTrivialInit
+
+void *operator new(unsigned long, void *);
+template 
+struct _box {
+  [[no_unique_address]] Ty _value;
+};
+// Make sure this doesn't crash.
+// CHECK-LABEL: define {{.*}}placement_new_struct
+void placement_new_struct() {
+  struct set_value_t {};
+
+  // GH88077
+  struct _tuple : _box, _box {};
+
+  int _storage[1];
+  new (_storage) _tuple{};
+
+  // GH89547
+  struct _tuple2 {
+_box a;
+  };
+
+  int _storage2[1];
+  new (_storage2) _tuple2{};
+}

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