[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Landed 44e2247dcd04f3421164b085094eb575270564ba 
 to fix 
LLDB. If you decide to go in a different direction again, please adjust that 
fix accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber 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 rG0f1137ba79c0: [clang/Basic] Make TargetInfo.h not use 
DataLayout again (authored by thakis).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D100776?vs=338558&id=341060#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100776

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/AST/Mangle.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/unittests/AST/DeclTest.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
  llvm/utils/gn/secondary/clang/tools/clang-format/BUILD.gn

Index: llvm/utils/gn/secondary/clang/tools/clang-format/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/tools/clang-format/BUILD.gn
+++ llvm/utils/gn/secondary/clang/tools/clang-format/BUILD.gn
@@ -11,6 +11,7 @@
 "//clang/lib/AST/",
 "//clang/lib/Frontend/",
 "//clang/lib/Sema/",
+"//llvm/lib/IR",
   ]
   sources = [ "ClangFormat.cpp" ]
 }
Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -49,7 +49,6 @@
 "//clang/include/clang/Sema:AttrParsedAttrKinds",
 "//clang/include/clang/Sema:AttrSpellingListIndex",
 "//llvm/include/llvm/Config:llvm-config",
-"//llvm/lib/IR",
 "//llvm/lib/MC",
 "//llvm/lib/Support",
   ]
Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -75,7 +75,7 @@
   auto AST =
   tooling::buildASTFromCodeWithArgs(Code, {"-target", "i386-apple-darwin"});
   ASTContext &Ctx = AST->getASTContext();
-  assert(Ctx.getTargetInfo().getDataLayout().getGlobalPrefix() &&
+  assert(Ctx.getTargetInfo().getUserLabelPrefix() == StringRef("_") &&
  "Expected target to have a global prefix");
   DiagnosticsEngine &Diags = AST->getDiagnostics();
 
@@ -118,7 +118,7 @@
   auto AST =
   tooling::buildASTFromCodeWithArgs(Code, {"-target", "i386-apple-darwin"});
   ASTContext &Ctx = AST->getASTContext();
-  assert(Ctx.getTargetInfo().getDataLayout().getGlobalPrefix() &&
+  assert(Ctx.getTargetInfo().getUserLabelPrefix() == StringRef("_") &&
  "Expected target to have a global prefix");
   DiagnosticsEngine &Diags = AST->getDiagnostics();
 
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -985,8 +985,7 @@
   DefineFastIntType(64, true, TI, Builder);
   DefineFastIntType(64, false, TI, Builder);
 
-  char UserLabelPrefix[2] = {TI.getDataLayout().getGlobalPrefix(), 0};
-  Builder.defineMacro("__USER_LABEL_PREFIX__", UserLabelPrefix);
+  Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());
 
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -166,7 +166,7 @@
 Ctx = &Context;
 VMContext.reset(new llvm::LLVMContext());
 M.reset(new llvm::Module(MainFileName, *VMContext));
-M->setDataLayout(Ctx->getTargetInfo().getDataLayout());
+M->setDataLayout(Ctx->getTargetInfo().getDataLayoutString());
 Builder.reset(new CodeGen::CodeGenModule(
 *Ctx, HeaderSearchOpts, PreprocessorOpts, CodeGenOpts, *M, Diags));
 
@@ -245,7 +245,7 @@
   return;
 
 M->setTargetTriple(Ctx.getTargetInfo().getTriple().getTriple());
-M->setDataLayout(Ctx.getTargetInfo().getDataLayout());
+M->setDataLayout(Ctx.getTargetInfo().getDataLayoutString());
 
 // PCH files don't have a signature field in the control block,
 // but LLVM detects DWO CUs by looking for a non-zero DWO id.
@@ -295,7 +295,7 @@
   llvm::SmallString<0> Buffer;
   clang::EmitBackendOutput(
   Diags, HeaderSearchOpts, CodeGenOpts, TargetOpts, LangOpts,
-  Ctx.getTargetInfo().getDataLayout(), M.get(),
+  

Re: [PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Eric Christopher via cfe-commits]

Sounds good. It's a soft objection, mostly because if nothing else it puts
us back where we were subject to some latent bugs, but perhaps not as bad
as before (though I don't find having to use an assert build reassuring ;)

Anyhow, go ahead and we'll figure out something else.

On Tue, Apr 27, 2021, 7:23 PM Reid Kleckner via Phabricator <
revi...@reviews.llvm.org> wrote:

> rnk accepted this revision.
> rnk added a comment.
> This revision is now accepted and ready to land.
>
> In D100776#2703273 , @echristo
> wrote:
>
> > As is mentioned there are tradeoffs around this though: a) it does make
> it harder to have clang generate code without a backend or llvm itself
> around, b) it does have a dependency when none existed.
> >
> > So, if this is really causing some consternation then we can pull back
> and reinstate what we had, but it was a direction around solving a set of
> hard to find bugs.
> >
> > Thoughts?
>
> I'm reading this as a soft, non-blocking objection. The concern that the
> layouts and prefix might get out of sync is addressed: there are asserts
> that they agree when the backends are linked in.
>
> So, under that interpretation, and without further guidance from
> @echristo, I think we should go forward. If that's not the right
> interpretation, we can always revert.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D100776/new/
>
> https://reviews.llvm.org/D100776
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Reid Kleckner via Phabricator via cfe-commits]

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

In D100776#2703273 , @echristo wrote:

> As is mentioned there are tradeoffs around this though: a) it does make it 
> harder to have clang generate code without a backend or llvm itself around, 
> b) it does have a dependency when none existed.
>
> So, if this is really causing some consternation then we can pull back and 
> reinstate what we had, but it was a direction around solving a set of hard to 
> find bugs.
>
> Thoughts?

I'm reading this as a soft, non-blocking objection. The concern that the 
layouts and prefix might get out of sync is addressed: there are asserts that 
they agree when the backends are linked in.

So, under that interpretation, and without further guidance from @echristo, I 
think we should go forward. If that's not the right interpretation, we can 
always revert.


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

ping?


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

rnk, echristo: can we go ahead here?


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> This keeps the assert that checks that the llvm DataLayout and the user label 
> prefix are in sync. See the NDEBUG in Mangle.cpp here-ish: 
> https://reviews.llvm.org/D100776#change-NN4Lz7xH29fo (I measured that that 
> assert doesn't slow down `check-clang` in a release+assert build too). So I 
> don't think this regresses safety.

It also keeps the easier-to-use resetDataLayout() API and sets the user label 
prefix as a 2nd (optional) arg in that function. That makes it harder to get 
them out of sync. And if you do get them out of sync, the assert will catch it 
as long as there's any test at all for that target. And if there isn't, you 
kind of have bigger problems :)


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> So, if this is really causing some consternation then we can pull back and 
> reinstate what we had, but it was a direction around solving a set of hard to 
> find bugs.
>
> Thoughts?

This keeps the assert that checks that the llvm DataLayout and the user label 
prefix are in sync. See the NDEBUG in Mangle.cpp here-ish: 
https://reviews.llvm.org/D100776#change-NN4Lz7xH29fo (I measured that that 
assert doesn't slow down `check-clang` in a release+assert build too). So I 
don't think this regresses safety.


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

In D100776#2699603 , @thakis wrote:

> In D100776#2699565 , @rnk wrote:
>
>> Of the three people who commented on D17183 
>> , you and I are on the only ones in favor 
>> of this approach. I think @echristo and @jyknight both preferred approach 2. 
>> I'd like to get at least an agreement to disagree from one of them before 
>> approving this.
>
> That's 50% in people, and 15/24 voting by changes that landed in clang/ since 
> start of year ;) Don't know why 2/4 is "only".

I mean that's fair ;)

> I'm also not dismissing approach 2: I implemented it as well, and it's 
> ready-to-go as soon as someone wants to pursue the direction mentioned there. 
> If someone wants to do that, I don't have a problem with it, but until then 
> this here is simpler and more self-consistent since it preserves the original 
> design of TargetInfo.

how about some historical context. I'm reasonably certain I was one of the 
motivators behind this change. The prior state often lead to either very subtle 
bugs that were hard to diagnose without an asserts build or a fairly 
inscrutable assert that the front and back ends disagreed on what context a 
string should have. The idea was that you could then ask the "backend" how to 
construct up a layout for what was wanted.

As is mentioned there are tradeoffs around this though: a) it does make it 
harder to have clang generate code without a backend or llvm itself around, b) 
it does have a dependency when none existed.

So, if this is really causing some consternation then we can pull back and 
reinstate what we had, but it was a direction around solving a set of hard to 
find bugs.

Thoughts?

-eric


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D100776#2699565 , @rnk wrote:

> Of the three people who commented on D17183 
> , you and I are on the only ones in favor of 
> this approach. I think @echristo and @jyknight both preferred approach 2. I'd 
> like to get at least an agreement to disagree from one of them before 
> approving this.

That's 50% in people, and 15/24 voting by changes that landed in clang/ since 
start of year ;) Don't know why 2/2 is "only".

I'm also not dismissing approach 2: I implemented it as well, and it's 
ready-to-go as soon as someone wants to pursue the direction mentioned there. 
If someone wants to do that, I don't have a problem with it, but until then 
this here is simpler and more self-consistent since it preserves the original 
design of TargetInfo.


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: jyknight, echristo.
rnk added a comment.

Of the three people who commented on D17183 , 
you and I are on the only ones in favor of this approach. I think @echristo and 
@jyknight both preferred approach 2. I'd like to get at least an agreement to 
disagree from one of them before approving this.

---

I remember now that the last time this came up, we wanted to preserve the 
ability for the frontend to generate IR for any target, even if it has no 
registered backend. This means we don't need `REQUIRES: foo-registered-target` 
lines in our clang IRgen tests. LLVM keeps the data layout in the backends. 
This dependency from clangBasic to LLVMIR wasn't a major consideration at the 
time. That makes me lean a little bit towards leaving this as it is.


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

https://reviews.llvm.org/D100776

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

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: rnk.
Herald added subscribers: dexonsmith, kerbowa, kbarton, mgorny, nhaehnle, 
jvesely, nemanjai.
thakis requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Reverts parts of https://reviews.llvm.org/D17183, but keeps the
resetDataLayout() API and adds an assert that checks that datalayout string and
user label prefix are in sync.

Approach 1 in https://reviews.llvm.org/D17183#2653279
Reduces number of TUs build for 'clang-format' from 689 to 575.

I also implemented approach 1 in D100764 . If 
someone feels motivated
to make us use DataLayout more, it's easy to revert this change here
and go with D100764  instead. I don't plan on 
doing more work in this
area though, so I prefer going with the smaller, more self-consistent change.


https://reviews.llvm.org/D100776

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/AST/Mangle.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/unittests/AST/DeclTest.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -45,10 +45,10 @@
 "//clang/include/clang/Sema:AttrParsedAttrKinds",
 "//clang/include/clang/Sema:AttrSpellingListIndex",
 "//llvm/include/llvm/Config:llvm-config",
-"//llvm/lib/IR",
 "//llvm/lib/MC",
 "//llvm/lib/Support",
   ]
+  assert_no_deps = [ "//llvm/lib/IR" ]
   include_dirs = [ "." ]
   sources = [
 "Attributes.cpp",
Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -75,7 +75,7 @@
   auto AST =
   tooling::buildASTFromCodeWithArgs(Code, {"-target", "i386-apple-darwin"});
   ASTContext &Ctx = AST->getASTContext();
-  assert(Ctx.getTargetInfo().getDataLayout().getGlobalPrefix() &&
+  assert(Ctx.getTargetInfo().getUserLabelPrefix() == StringRef("_") &&
  "Expected target to have a global prefix");
   DiagnosticsEngine &Diags = AST->getDiagnostics();
 
@@ -118,7 +118,7 @@
   auto AST =
   tooling::buildASTFromCodeWithArgs(Code, {"-target", "i386-apple-darwin"});
   ASTContext &Ctx = AST->getASTContext();
-  assert(Ctx.getTargetInfo().getDataLayout().getGlobalPrefix() &&
+  assert(Ctx.getTargetInfo().getUserLabelPrefix() == StringRef("_") &&
  "Expected target to have a global prefix");
   DiagnosticsEngine &Diags = AST->getDiagnostics();
 
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -985,8 +985,7 @@
   DefineFastIntType(64, true, TI, Builder);
   DefineFastIntType(64, false, TI, Builder);
 
-  char UserLabelPrefix[2] = {TI.getDataLayout().getGlobalPrefix(), 0};
-  Builder.defineMacro("__USER_LABEL_PREFIX__", UserLabelPrefix);
+  Builder.defineMacro("__USER_LABEL_PREFIX__", TI.getUserLabelPrefix());
 
   if (LangOpts.FastMath || LangOpts.FiniteMathOnly)
 Builder.defineMacro("__FINITE_MATH_ONLY__", "1");
Index: clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
===
--- clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
+++ clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
@@ -166,7 +166,7 @@
 Ctx = &Context;
 VMContext.reset(new llvm::LLVMContext());
 M.reset(new llvm::Module(MainFileName, *VMContext));
-M->setDataLayout(Ctx->getTargetInfo().getDataLayout());
+M->setDataLayout(Ctx->getTargetInfo().getDataLayoutString());
 Builder.reset(new CodeGen::CodeGenModule(
 *Ctx, HeaderSearchOpts, PreprocessorOpts, CodeGenOpts, *M, Diags));
 
@@ -245,7 +245,7 @@
   return;
 
 M->setTargetTriple(Ctx.getTargetInfo().getTriple().getTriple());
-M->setDataLayout(Ctx.getTargetInfo().getDataLayout());
+M->setDataLayout(Ctx.getTargetInfo().getDataLayoutString());
 
 // PCH files don't have a signature field in the control block,
 // but LLVM detects DWO CUs by looking for a non-zero DWO id.
@@ -295,7 +295,7 @@
   llvm::SmallString<0> Buffer;
   clang::EmitBackendOutput(
   Diags, HeaderSearchOpts, CodeGenOpts, TargetOpt