[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

davezarzycki wrote:
> aaron.ballman wrote:
> > davezarzycki wrote:
> > > aaron.ballman wrote:
> > > > davezarzycki wrote:
> > > > > aaron.ballman wrote:
> > > > > > rnk wrote:
> > > > > > > This includes Sema.h into every codegen file that uses CGValue.h 
> > > > > > > (most of them). That seems bad for build time. :(
> > > > > > > 
> > > > > > > This also seems like a layering violation. CodeGen has no 
> > > > > > > dependency on Sema:
> > > > > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > > > > > I agree that this is a layering violation (Sema relies on CodeGen 
> > > > > > which now relies on Sema due to this change). We just ran into it 
> > > > > > in a downstream fork when we had to add `clangSema` to the codegen 
> > > > > > linker input to avoid linking errors. I'm a bit surprised given 
> > > > > > that the only use appears to be a `static_assert` that shouldn't 
> > > > > > require anything to be linked in, but here we are just the same.
> > > > > > 
> > > > > > I think this should be rolled back so that we don't get additional 
> > > > > > dependencies on Sema within CodeGen by accident. It helps that @rnk 
> > > > > > moved this change into CGDecl.cpp (limits the scope of where we may 
> > > > > > introduce accidental dependencies), but I don't think we should be 
> > > > > > including anything from Sema.h here.
> > > > > It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
> > > > That fixes the transitive include issues @rnk raised, but is not a fix 
> > > > for the layering violation. It's still a layering violation because 
> > > > it's including Sema from within CodeGen.
> > > Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> > > trigger this. Did you verify that the imported symbol dependency is 
> > > triggered by this static_assert? Or is the problem the mere inclusion, 
> > > not the static_assert itself? And is the actual dependency only happen in 
> > > unoptimized/debug builds or release too?
> > > 
> > > In any case, I believe it was discussed at the time that the LLVM 
> > > variable ought to be moved to some kind of policy/config header that both 
> > > LLVM's CodeGen and clang's Sema can include. Would you be open to that?
> > > Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> > > trigger this. Did you verify that the imported symbol dependency is 
> > > triggered by this static_assert? Or is the problem the mere inclusion, 
> > > not the static_assert itself? And is the actual dependency only happen in 
> > > unoptimized/debug builds or release too?
> > 
> > I'd have to go back to the developer who found it to get the exact details, 
> > but my belief is that it's the inclusion more so than the static assertion 
> > itself. FWIW, the dependency was happening when doing a shared libraries 
> > build (I believe in release mode). I can dig up those details if you'd 
> > like, but my concern is that even if the issue is the static assertion, 
> > including the header file makes it easy for someone to accidentally use 
> > other facilities from Sema.h under the mistaken belief they're fine to do 
> > so.
> > 
> > > In any case, I believe it was discussed at the time that the LLVM 
> > > variable ought to be moved to some kind of policy/config header that both 
> > > LLVM's CodeGen and clang's Sema can include. Would you be open to that?
> > 
> > That sounds like a very good idea to me!
> Okay. I sent out the RFC to llvm-dev. It was long overdue. Somebody just 
> needed to volunteer, start the discussion, and get it over with.
Thank you for your help on this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

aaron.ballman wrote:
> davezarzycki wrote:
> > aaron.ballman wrote:
> > > davezarzycki wrote:
> > > > aaron.ballman wrote:
> > > > > rnk wrote:
> > > > > > This includes Sema.h into every codegen file that uses CGValue.h 
> > > > > > (most of them). That seems bad for build time. :(
> > > > > > 
> > > > > > This also seems like a layering violation. CodeGen has no 
> > > > > > dependency on Sema:
> > > > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > > > > I agree that this is a layering violation (Sema relies on CodeGen 
> > > > > which now relies on Sema due to this change). We just ran into it in 
> > > > > a downstream fork when we had to add `clangSema` to the codegen 
> > > > > linker input to avoid linking errors. I'm a bit surprised given that 
> > > > > the only use appears to be a `static_assert` that shouldn't require 
> > > > > anything to be linked in, but here we are just the same.
> > > > > 
> > > > > I think this should be rolled back so that we don't get additional 
> > > > > dependencies on Sema within CodeGen by accident. It helps that @rnk 
> > > > > moved this change into CGDecl.cpp (limits the scope of where we may 
> > > > > introduce accidental dependencies), but I don't think we should be 
> > > > > including anything from Sema.h here.
> > > > It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
> > > That fixes the transitive include issues @rnk raised, but is not a fix 
> > > for the layering violation. It's still a layering violation because it's 
> > > including Sema from within CodeGen.
> > Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> > trigger this. Did you verify that the imported symbol dependency is 
> > triggered by this static_assert? Or is the problem the mere inclusion, not 
> > the static_assert itself? And is the actual dependency only happen in 
> > unoptimized/debug builds or release too?
> > 
> > In any case, I believe it was discussed at the time that the LLVM variable 
> > ought to be moved to some kind of policy/config header that both LLVM's 
> > CodeGen and clang's Sema can include. Would you be open to that?
> > Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> > trigger this. Did you verify that the imported symbol dependency is 
> > triggered by this static_assert? Or is the problem the mere inclusion, not 
> > the static_assert itself? And is the actual dependency only happen in 
> > unoptimized/debug builds or release too?
> 
> I'd have to go back to the developer who found it to get the exact details, 
> but my belief is that it's the inclusion more so than the static assertion 
> itself. FWIW, the dependency was happening when doing a shared libraries 
> build (I believe in release mode). I can dig up those details if you'd like, 
> but my concern is that even if the issue is the static assertion, including 
> the header file makes it easy for someone to accidentally use other 
> facilities from Sema.h under the mistaken belief they're fine to do so.
> 
> > In any case, I believe it was discussed at the time that the LLVM variable 
> > ought to be moved to some kind of policy/config header that both LLVM's 
> > CodeGen and clang's Sema can include. Would you be open to that?
> 
> That sounds like a very good idea to me!
Okay. I sent out the RFC to llvm-dev. It was long overdue. Somebody just needed 
to volunteer, start the discussion, and get it over with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

davezarzycki wrote:
> aaron.ballman wrote:
> > davezarzycki wrote:
> > > aaron.ballman wrote:
> > > > rnk wrote:
> > > > > This includes Sema.h into every codegen file that uses CGValue.h 
> > > > > (most of them). That seems bad for build time. :(
> > > > > 
> > > > > This also seems like a layering violation. CodeGen has no dependency 
> > > > > on Sema:
> > > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > > > I agree that this is a layering violation (Sema relies on CodeGen which 
> > > > now relies on Sema due to this change). We just ran into it in a 
> > > > downstream fork when we had to add `clangSema` to the codegen linker 
> > > > input to avoid linking errors. I'm a bit surprised given that the only 
> > > > use appears to be a `static_assert` that shouldn't require anything to 
> > > > be linked in, but here we are just the same.
> > > > 
> > > > I think this should be rolled back so that we don't get additional 
> > > > dependencies on Sema within CodeGen by accident. It helps that @rnk 
> > > > moved this change into CGDecl.cpp (limits the scope of where we may 
> > > > introduce accidental dependencies), but I don't think we should be 
> > > > including anything from Sema.h here.
> > > It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
> > That fixes the transitive include issues @rnk raised, but is not a fix for 
> > the layering violation. It's still a layering violation because it's 
> > including Sema from within CodeGen.
> Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> trigger this. Did you verify that the imported symbol dependency is triggered 
> by this static_assert? Or is the problem the mere inclusion, not the 
> static_assert itself? And is the actual dependency only happen in 
> unoptimized/debug builds or release too?
> 
> In any case, I believe it was discussed at the time that the LLVM variable 
> ought to be moved to some kind of policy/config header that both LLVM's 
> CodeGen and clang's Sema can include. Would you be open to that?
> Ah, sorry. It does seem odd and probably a bug that a static_assert would 
> trigger this. Did you verify that the imported symbol dependency is triggered 
> by this static_assert? Or is the problem the mere inclusion, not the 
> static_assert itself? And is the actual dependency only happen in 
> unoptimized/debug builds or release too?

I'd have to go back to the developer who found it to get the exact details, but 
my belief is that it's the inclusion more so than the static assertion itself. 
FWIW, the dependency was happening when doing a shared libraries build (I 
believe in release mode). I can dig up those details if you'd like, but my 
concern is that even if the issue is the static assertion, including the header 
file makes it easy for someone to accidentally use other facilities from Sema.h 
under the mistaken belief they're fine to do so.

> In any case, I believe it was discussed at the time that the LLVM variable 
> ought to be moved to some kind of policy/config header that both LLVM's 
> CodeGen and clang's Sema can include. Would you be open to that?

That sounds like a very good idea to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

aaron.ballman wrote:
> davezarzycki wrote:
> > aaron.ballman wrote:
> > > rnk wrote:
> > > > This includes Sema.h into every codegen file that uses CGValue.h (most 
> > > > of them). That seems bad for build time. :(
> > > > 
> > > > This also seems like a layering violation. CodeGen has no dependency on 
> > > > Sema:
> > > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > > I agree that this is a layering violation (Sema relies on CodeGen which 
> > > now relies on Sema due to this change). We just ran into it in a 
> > > downstream fork when we had to add `clangSema` to the codegen linker 
> > > input to avoid linking errors. I'm a bit surprised given that the only 
> > > use appears to be a `static_assert` that shouldn't require anything to be 
> > > linked in, but here we are just the same.
> > > 
> > > I think this should be rolled back so that we don't get additional 
> > > dependencies on Sema within CodeGen by accident. It helps that @rnk moved 
> > > this change into CGDecl.cpp (limits the scope of where we may introduce 
> > > accidental dependencies), but I don't think we should be including 
> > > anything from Sema.h here.
> > It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
> That fixes the transitive include issues @rnk raised, but is not a fix for 
> the layering violation. It's still a layering violation because it's 
> including Sema from within CodeGen.
Ah, sorry. It does seem odd and probably a bug that a static_assert would 
trigger this. Did you verify that the imported symbol dependency is triggered 
by this static_assert? Or is the problem the mere inclusion, not the 
static_assert itself? And is the actual dependency only happen in 
unoptimized/debug builds or release too?

In any case, I believe it was discussed at the time that the LLVM variable 
ought to be moved to some kind of policy/config header that both LLVM's CodeGen 
and clang's Sema can include. Would you be open to that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

davezarzycki wrote:
> aaron.ballman wrote:
> > rnk wrote:
> > > This includes Sema.h into every codegen file that uses CGValue.h (most of 
> > > them). That seems bad for build time. :(
> > > 
> > > This also seems like a layering violation. CodeGen has no dependency on 
> > > Sema:
> > > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> > I agree that this is a layering violation (Sema relies on CodeGen which now 
> > relies on Sema due to this change). We just ran into it in a downstream 
> > fork when we had to add `clangSema` to the codegen linker input to avoid 
> > linking errors. I'm a bit surprised given that the only use appears to be a 
> > `static_assert` that shouldn't require anything to be linked in, but here 
> > we are just the same.
> > 
> > I think this should be rolled back so that we don't get additional 
> > dependencies on Sema within CodeGen by accident. It helps that @rnk moved 
> > this change into CGDecl.cpp (limits the scope of where we may introduce 
> > accidental dependencies), but I don't think we should be including anything 
> > from Sema.h here.
> It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13
That fixes the transitive include issues @rnk raised, but is not a fix for the 
layering violation. It's still a layering violation because it's including Sema 
from within CodeGen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

aaron.ballman wrote:
> rnk wrote:
> > This includes Sema.h into every codegen file that uses CGValue.h (most of 
> > them). That seems bad for build time. :(
> > 
> > This also seems like a layering violation. CodeGen has no dependency on 
> > Sema:
> > https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
> I agree that this is a layering violation (Sema relies on CodeGen which now 
> relies on Sema due to this change). We just ran into it in a downstream fork 
> when we had to add `clangSema` to the codegen linker input to avoid linking 
> errors. I'm a bit surprised given that the only use appears to be a 
> `static_assert` that shouldn't require anything to be linked in, but here we 
> are just the same.
> 
> I think this should be rolled back so that we don't get additional 
> dependencies on Sema within CodeGen by accident. It helps that @rnk moved 
> this change into CGDecl.cpp (limits the scope of where we may introduce 
> accidental dependencies), but I don't think we should be including anything 
> from Sema.h here.
It was fixed over a year ago: 01943a59f51d8b5ede062305941c1f864b8a6a13


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

rnk wrote:
> This includes Sema.h into every codegen file that uses CGValue.h (most of 
> them). That seems bad for build time. :(
> 
> This also seems like a layering violation. CodeGen has no dependency on Sema:
> https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104
I agree that this is a layering violation (Sema relies on CodeGen which now 
relies on Sema due to this change). We just ran into it in a downstream fork 
when we had to add `clangSema` to the codegen linker input to avoid linking 
errors. I'm a bit surprised given that the only use appears to be a 
`static_assert` that shouldn't require anything to be linked in, but here we 
are just the same.

I think this should be rolled back so that we don't get additional dependencies 
on Sema within CodeGen by accident. It helps that @rnk moved this change into 
CGDecl.cpp (limits the scope of where we may introduce accidental 
dependencies), but I don't think we should be including anything from Sema.h 
here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I moved this include in rG01943a59f51d8b5ede062305941c1f864b8a6a13 
. I meant 
to paste this in the message, but I'll put it here, since the results were 
significant:

  $ diff -u deps-before.txt deps-after.txt |  grep '^[-+] ' |  sort | uniq -c | 
sort -nr
   50 -../clang/include/clang/Sema/Weak.h
   50 -../clang/include/clang/Sema/TypoCorrection.h
   50 -../clang/include/clang/Sema/SemaConcept.h
   50 -../clang/include/clang/Sema/Sema.h
   50 -../clang/include/clang/Sema/Scope.h
   50 -../clang/include/clang/Sema/ObjCMethodList.h
   50 -../clang/include/clang/Sema/IdentifierResolver.h
   50 -../clang/include/clang/Sema/ExternalSemaSource.h
   50 -../clang/include/clang/Sema/CleanupInfo.h
   50 -../clang/include/clang/Sema/AnalysisBasedWarnings.h
   50 -../clang/include/clang/Basic/TemplateKinds.h
   50 -../clang/include/clang/AST/MangleNumberingContext.h
   50 -../clang/include/clang/AST/LocInfoType.h
   50 -../clang/include/clang/AST/Availability.h
   49 -tools/clang/include/clang/Basic/AttrSubMatchRulesList.inc
   49 -../llvm/include/llvm/ADT/SmallBitVector.h
   49 -../clang/include/clang/Sema/ParsedAttr.h
   49 -../clang/include/clang/Sema/Ownership.h
   49 -../clang/include/clang/Sema/DeclSpec.h
   49 -../clang/include/clang/Basic/BitmaskEnum.h
   49 -../clang/include/clang/Basic/AttrSubjectMatchRules.h
   49 -../clang/include/clang/AST/NSAPI.h
   47 -../clang/include/clang/Lex/Token.h
   36 -../clang/include/clang/AST/ExprConcepts.h
   31 -../clang/include/clang/AST/StmtCXX.h
   21 -tools/clang/include/clang/AST/Attrs.inc
   21 -../clang/include/clang/AST/Attr.h
   20 -tools/clang/include/clang/Sema/AttrParsedAttrList.inc
   20 -../clang/include/clang/Basic/AttributeCommonInfo.h
7 -../clang/include/clang/Basic/ExpressionTraits.h
7 -../clang/include/clang/AST/ExprObjC.h
7 -../clang/include/clang/AST/ExprCXX.h
7 -../clang/include/clang/AST/DeclTemplate.h
7 -../clang/include/clang/AST/ASTConcept.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363



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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGValue.h:18
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"

This includes Sema.h into every codegen file that uses CGValue.h (most of 
them). That seems bad for build time. :(

This also seems like a layering violation. CodeGen has no dependency on Sema:
https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CMakeLists.txt#L104


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363



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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0d61cd25a692: Verify that clangs max alignment is 
= LLVMs max alignment (authored by davezarzycki).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGValue.h


Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -15,12 +15,16 @@
 #define LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/Type.h"
 #include "Address.h"
 #include "CodeGenTBAA.h"
 
+static_assert(clang::Sema::MaximumAlignment <= llvm::Value::MaximumAlignment,
+  "Clang max alignment greater than what LLVM supports?");
+
 namespace llvm {
   class Constant;
   class MDNode;
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -372,16 +372,17 @@
   QualType ResultTy,
   ArrayRef Args);
 
+public:
   /// The maximum alignment, same as in llvm::Value. We duplicate them here
   /// because that allows us not to duplicate the constants in clang code,
   /// which we must to since we can't directly use the llvm constants.
+  /// The value is verified against llvm here: lib/CodeGen/CGValue.h
   ///
   /// This is the greatest alignment value supported by load, store, and alloca
   /// instructions, and global values.
   static const unsigned MaxAlignmentExponent = 29;
   static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
 
-public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;
   typedef OpaquePtr TypeTy;


Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -15,12 +15,16 @@
 #define LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/Type.h"
 #include "Address.h"
 #include "CodeGenTBAA.h"
 
+static_assert(clang::Sema::MaximumAlignment <= llvm::Value::MaximumAlignment,
+  "Clang max alignment greater than what LLVM supports?");
+
 namespace llvm {
   class Constant;
   class MDNode;
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -372,16 +372,17 @@
   QualType ResultTy,
   ArrayRef Args);
 
+public:
   /// The maximum alignment, same as in llvm::Value. We duplicate them here
   /// because that allows us not to duplicate the constants in clang code,
   /// which we must to since we can't directly use the llvm constants.
+  /// The value is verified against llvm here: lib/CodeGen/CGValue.h
   ///
   /// This is the greatest alignment value supported by load, store, and alloca
   /// instructions, and global values.
   static const unsigned MaxAlignmentExponent = 29;
   static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
 
-public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;
   typedef OpaquePtr TypeTy;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[Roman Lebedev via Phabricator via cfe-commits]

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

SGTM, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73363



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

[PATCH] D73363: Verify that clang's max alignment is <= LLVM's max alignment

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki created this revision.
davezarzycki added a reviewer: lebedev.ri.
davezarzycki added projects: LLVM, clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73363

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGValue.h


Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -15,12 +15,16 @@
 #define LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/Type.h"
 #include "Address.h"
 #include "CodeGenTBAA.h"
 
+static_assert(clang::Sema::MaximumAlignment <= llvm::Value::MaximumAlignment,
+  "Clang max alignment greater than what LLVM supports?");
+
 namespace llvm {
   class Constant;
   class MDNode;
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -372,16 +372,17 @@
   QualType ResultTy,
   ArrayRef Args);
 
+public:
   /// The maximum alignment, same as in llvm::Value. We duplicate them here
   /// because that allows us not to duplicate the constants in clang code,
   /// which we must to since we can't directly use the llvm constants.
+  /// The value is verified against llvm here: lib/CodeGen/CGValue.h
   ///
   /// This is the greatest alignment value supported by load, store, and alloca
   /// instructions, and global values.
   static const unsigned MaxAlignmentExponent = 29;
   static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
 
-public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;
   typedef OpaquePtr TypeTy;


Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -15,12 +15,16 @@
 #define LLVM_CLANG_LIB_CODEGEN_CGVALUE_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Sema/Sema.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/IR/Type.h"
 #include "Address.h"
 #include "CodeGenTBAA.h"
 
+static_assert(clang::Sema::MaximumAlignment <= llvm::Value::MaximumAlignment,
+  "Clang max alignment greater than what LLVM supports?");
+
 namespace llvm {
   class Constant;
   class MDNode;
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -372,16 +372,17 @@
   QualType ResultTy,
   ArrayRef Args);
 
+public:
   /// The maximum alignment, same as in llvm::Value. We duplicate them here
   /// because that allows us not to duplicate the constants in clang code,
   /// which we must to since we can't directly use the llvm constants.
+  /// The value is verified against llvm here: lib/CodeGen/CGValue.h
   ///
   /// This is the greatest alignment value supported by load, store, and alloca
   /// instructions, and global values.
   static const unsigned MaxAlignmentExponent = 29;
   static const unsigned MaximumAlignment = 1u << MaxAlignmentExponent;
 
-public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;
   typedef OpaquePtr TypeTy;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits