[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

akhuang closed this revision.
akhuang added a comment.

ah sorry, this was relanded in b1009ee84fc0242bcebd07889306bf39d9b7170f 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Looks good, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

akhuang updated this revision to Diff 287748.
akhuang added a comment.

Fix errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=limited %s -o - | FileCheck %s
 
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=constructor %s -o - | FileCheck %s
+
 // Run again with -gline-tables-only or -gline-directives-only and verify we 
don't crash.  We won't output
 // type info at all.
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=line-tables-only %s -o - | FileCheck %s -check-prefix 
LINES-ONLY
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  // dllimport.
+  if (RD->isLambda() || RD->hasConstexprNonCopyMoveConstructor() ||
+  isClassOrMethodDLLImport(RD))
+return false;
+
+  if (RD->ctors().empty())
+return false;
+
+  for (const auto *Ctor : RD->ctors())
+if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
+  return false;
+
+  return true;
+}
+
 static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
  bool DebugTypeExtRefs, const RecordDecl *RD,
  const LangOptions ) {
@@ -2294,23 +2313,6 @@
   !isClassOrMethodDLLImport(CXXDecl))
 return true;
 
-  // In constructor debug mode, only emit debug info for a class when its
-  // constructor is emitted. Skip this optimization if the class or any of
-  // its methods are marked dllimport.
-  //
-  // This applies to classes that don't have any trivial constructors and have
-  // at least one constructor.
-  if (DebugKind == codegenoptions::DebugInfoConstructor &&
-  !CXXDecl->isLambda() && !CXXDecl->hasConstexprNonCopyMoveConstructor() &&
-  !isClassOrMethodDLLImport(CXXDecl)) {
-if (CXXDecl->ctors().empty())
-  return false;
-for (const auto *Ctor : CXXDecl->ctors())
-  if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
-return false;
-return true;
-  }
-
   TemplateSpecializationKind Spec = TSK_Undeclared;
   if (const auto *SD = dyn_cast(RD))
 Spec = SD->getSpecializationKind();
@@ -2320,6 +2322,12 @@
   CXXDecl->method_end()))
 return true;
 
+  // In constructor homing mode, only emit complete debug info for a class
+  // when its constructor is emitted.
+  if ((DebugKind == codegenoptions::DebugInfoConstructor) &&
+  canUseCtorHoming(CXXDecl))
+return true;
+
   return false;
 }
 


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %s
 
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=constructor %s -o - | FileCheck %s
+
 // Run again with -gline-tables-only or -gline-directives-only and verify we don't crash.  We won't output
 // type info at all.
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=line-tables-only %s -o - | FileCheck %s -check-prefix LINES-ONLY
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  // dllimport.
+  if (RD->isLambda() || RD->hasConstexprNonCopyMoveConstructor() ||
+  

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

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

just reopening to update the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

akhuang added a comment.

In D86491#2235206 , @MaskRay wrote:

> Note that Harbormaster actually reported the check-clang-codegencxx issues 
> B69369 ..

Definitely my bad; I thought I had run check-clang locally before committing, 
but maybe .. I didn't?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Note that Harbormaster actually reported the check-clang-codegencxx issues 
B69369 ..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang 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 rG589ce5f7050d: [DebugInfo] Move constructor homing case in 
shouldOmitDefinition. (authored by akhuang).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=limited %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=limited %s -o - | FileCheck %
+
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=constructor %s -o - | FileCheck %s
 
 // Run again with -gline-tables-only or -gline-directives-only and verify we 
don't crash.  We won't output
 // type info at all.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  // dllimport.
+  if (RD->isLambda() || RD->hasConstexprNonCopyMoveConstructor() ||
+  isClassOrMethodDLLImport(RD))
+return false;
+
+  if (RD->ctors().empty())
+return false;
+
+  for (const auto *Ctor : RD->ctors())
+if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
+  return false;
+
+  return true;
+}
+
 static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
  bool DebugTypeExtRefs, const RecordDecl *RD,
  const LangOptions ) {
@@ -2294,23 +2313,6 @@
   !isClassOrMethodDLLImport(CXXDecl))
 return true;
 
-  // In constructor debug mode, only emit debug info for a class when its
-  // constructor is emitted. Skip this optimization if the class or any of
-  // its methods are marked dllimport.
-  //
-  // This applies to classes that don't have any trivial constructors and have
-  // at least one constructor.
-  if (DebugKind == codegenoptions::DebugInfoConstructor &&
-  !CXXDecl->isLambda() && !CXXDecl->hasConstexprNonCopyMoveConstructor() &&
-  !isClassOrMethodDLLImport(CXXDecl)) {
-if (CXXDecl->ctors().empty())
-  return false;
-for (const auto *Ctor : CXXDecl->ctors())
-  if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
-return false;
-return true;
-  }
-
   TemplateSpecializationKind Spec = TSK_Undeclared;
   if (const auto *SD = dyn_cast(RD))
 Spec = SD->getSpecializationKind();
@@ -2320,6 +2322,12 @@
   CXXDecl->method_end()))
 return true;
 
+  // In constructor homing mode, only emit complete debug info for a class
+  // when its constructor is emitted.
+  if ((DebugKind != codegenoptions::DebugInfoConstructor) &&
+  canUseCtorHoming(CXXDecl))
+return true;
+
   return false;
 }
 


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %
+
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=constructor %s -o - | FileCheck %s
 
 // Run again with -gline-tables-only or -gline-directives-only and verify we don't crash.  We won't output
 // type info at all.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

akhuang added a comment.

unfortunately not any thorough testing :)  I just happened to notice it the 
last time I looked at this code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[David Blaikie via Phabricator via cfe-commits]

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

Thanks for testing/catching this (what sort of testing have you been doing that 
discovered this?)!

& yeah, the way shouldOmitDefinition is now written (with this fix) is right - 
that every check only gives an opportunity to return true, then falls through 
to the next case - never an early return false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86491

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

[PATCH] D86491: [DebugInfo] Move constructor homing case in shouldOmitDefinition.

[Amy Huang via Phabricator via cfe-commits]

akhuang created this revision.
akhuang added a reviewer: dblaikie.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
akhuang requested review of this revision.

For some reason the ctor homing case was before the template
specialization case, and could have returned false too early.
I moved the code out into a separate function to avoid this.

Also added a run line to the template specialization test. I guess
all the -debug-info-kind=limited tests should still pass with =constructor,
but it's probably unnecessary to test for all of those.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86491

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=limited %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=limited %s -o - | FileCheck %
+
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple 
-debug-info-kind=constructor %s -o - | FileCheck %s
 
 // Run again with -gline-tables-only or -gline-directives-only and verify we 
don't crash.  We won't output
 // type info at all.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool canUseCtorHoming(const CXXRecordDecl *RD) {
+  // Constructor homing can be used for classes that have at least one
+  // constructor and have no trivial or constexpr constructors.
+  // Skip this optimization if the class or any of its methods are marked
+  // dllimport.
+  if (RD->isLambda() || RD->hasConstexprNonCopyMoveConstructor() ||
+  isClassOrMethodDLLImport(RD))
+return false;
+
+  if (RD->ctors().empty())
+return false;
+
+  for (const auto *Ctor : RD->ctors())
+if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
+  return false;
+
+  return true;
+}
+
 static bool shouldOmitDefinition(codegenoptions::DebugInfoKind DebugKind,
  bool DebugTypeExtRefs, const RecordDecl *RD,
  const LangOptions ) {
@@ -2294,23 +2313,6 @@
   !isClassOrMethodDLLImport(CXXDecl))
 return true;
 
-  // In constructor debug mode, only emit debug info for a class when its
-  // constructor is emitted. Skip this optimization if the class or any of
-  // its methods are marked dllimport.
-  //
-  // This applies to classes that don't have any trivial constructors and have
-  // at least one constructor.
-  if (DebugKind == codegenoptions::DebugInfoConstructor &&
-  !CXXDecl->isLambda() && !CXXDecl->hasConstexprNonCopyMoveConstructor() &&
-  !isClassOrMethodDLLImport(CXXDecl)) {
-if (CXXDecl->ctors().empty())
-  return false;
-for (const auto *Ctor : CXXDecl->ctors())
-  if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
-return false;
-return true;
-  }
-
   TemplateSpecializationKind Spec = TSK_Undeclared;
   if (const auto *SD = dyn_cast(RD))
 Spec = SD->getSpecializationKind();
@@ -2320,6 +2322,12 @@
   CXXDecl->method_end()))
 return true;
 
+  // In constructor homing mode, only emit complete debug info for a class
+  // when its constructor is emitted.
+  if ((DebugKind != codegenoptions::DebugInfoConstructor) &&
+  canUseCtorHoming(CXXDecl))
+return true;
+
   return false;
 }
 


Index: clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
@@ -1,4 +1,7 @@
-// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited %s -o - | FileCheck %
+
+// Make sure this still works with constructor homing.
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=constructor %s -o - | FileCheck %s
 
 // Run again with -gline-tables-only or -gline-directives-only and verify we don't crash.  We won't output
 // type info at all.
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2260,6 +2260,25 @@
   return false;
 }
 
+static bool