[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations
erichkeane added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:2154
+ if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType())
+.TryEval(Attr->getAlignmentExpr())) {
+TI.Align = Result;
aaron.ballman wrote:
> rsmith wrote:
> > I don't think this approach is going to work out well. There are many cases
> > this gets wrong, fixing them would require reinventing template
> > substitution here, and generally we shouldn't be doing this substitution as
> > part of alignment computation at all -- we should have `TreeTransform`
> > produce the right alignment value during template instantiation and just
> > pull it back out of the `Type` here. We can't really use the same approach
> > as we do for `TypedefDecl`s, though, because we don't instantiate a
> > `TypeAliasDecl` for each use of a `TypeAliasTemplateDecl`, so there's
> > nowhere to hang an instantiated attribute. But the handling for
> > `TypedefType`s is also fairly painful, so I think we should be considering
> > an approach that makes both of them more elegant. Here's what I suggest:
> >
> > - We add a new sugar-only `Type` subclass that represents an alignment
> > attribute. Maybe we can model this as an `AttributedType` for the
> > `AlignedAttr`, or maybe we create a new kind of type node.
> > - We translate `AlignedAttr`s on typedef and type alias declarations into
> > this new kind of `Type` wrapping the declared type.
> > - We make `getTypeInfoImpl` special-case that type sugar node instead of
> > special-casing `TypedefType` sugar.
> > - We make sure that `TreeTransform` properly instantiates the new node, in
> > particular performing substitution within the argument.
> I think this approach makes sense to me, but it's worth noting: the alignment
> attributes (vendor specific or standard) are declaration attributes and not
> type attributes. I think this is a design mistake; alignment is a fundamental
> property of a type (see http://eel.is/c++draft/basic.align#1), so I'm in
> favor of modeling this as a type attribute, but the standard `alignas`
> attribute does not have type semantics: http://eel.is/c++draft/dcl.align#1.
> Should we approach the standards committees about this or are we not
> concerned about `alignas` behavior?
>
> (If we expect `alignas` to be a declaration attribute and `[[gnu::aligned]]`
> to be a type attribute, another question is whether we should split them in
> Attr.td.)
IMO, the idea of doing this via this visitor is incorrect, we should just make
sure the instantiated decl of the type alias to get the attribute, we do
similar things for function decls.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143533/new/
https://reviews.llvm.org/D143533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations
aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.
Adding Erich as attributes code owner.
Comment at: clang/lib/AST/ASTContext.cpp:2154
+ if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType())
+.TryEval(Attr->getAlignmentExpr())) {
+TI.Align = Result;
rsmith wrote:
> I don't think this approach is going to work out well. There are many cases
> this gets wrong, fixing them would require reinventing template substitution
> here, and generally we shouldn't be doing this substitution as part of
> alignment computation at all -- we should have `TreeTransform` produce the
> right alignment value during template instantiation and just pull it back out
> of the `Type` here. We can't really use the same approach as we do for
> `TypedefDecl`s, though, because we don't instantiate a `TypeAliasDecl` for
> each use of a `TypeAliasTemplateDecl`, so there's nowhere to hang an
> instantiated attribute. But the handling for `TypedefType`s is also fairly
> painful, so I think we should be considering an approach that makes both of
> them more elegant. Here's what I suggest:
>
> - We add a new sugar-only `Type` subclass that represents an alignment
> attribute. Maybe we can model this as an `AttributedType` for the
> `AlignedAttr`, or maybe we create a new kind of type node.
> - We translate `AlignedAttr`s on typedef and type alias declarations into
> this new kind of `Type` wrapping the declared type.
> - We make `getTypeInfoImpl` special-case that type sugar node instead of
> special-casing `TypedefType` sugar.
> - We make sure that `TreeTransform` properly instantiates the new node, in
> particular performing substitution within the argument.
I think this approach makes sense to me, but it's worth noting: the alignment
attributes (vendor specific or standard) are declaration attributes and not
type attributes. I think this is a design mistake; alignment is a fundamental
property of a type (see http://eel.is/c++draft/basic.align#1), so I'm in favor
of modeling this as a type attribute, but the standard `alignas` attribute does
not have type semantics: http://eel.is/c++draft/dcl.align#1. Should we approach
the standards committees about this or are we not concerned about `alignas`
behavior?
(If we expect `alignas` to be a declaration attribute and `[[gnu::aligned]]` to
be a type attribute, another question is whether we should split them in
Attr.td.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143533/new/
https://reviews.llvm.org/D143533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations
rsmith added inline comments.
Comment at: clang/lib/AST/ASTContext.cpp:2154
+ if (uint64_t Result = SubstTypeVisitor(Ctx, SubType->getReplacementType())
+.TryEval(Attr->getAlignmentExpr())) {
+TI.Align = Result;
I don't think this approach is going to work out well. There are many cases
this gets wrong, fixing them would require reinventing template substitution
here, and generally we shouldn't be doing this substitution as part of
alignment computation at all -- we should have `TreeTransform` produce the
right alignment value during template instantiation and just pull it back out
of the `Type` here. We can't really use the same approach as we do for
`TypedefDecl`s, though, because we don't instantiate a `TypeAliasDecl` for each
use of a `TypeAliasTemplateDecl`, so there's nowhere to hang an instantiated
attribute. But the handling for `TypedefType`s is also fairly painful, so I
think we should be considering an approach that makes both of them more
elegant. Here's what I suggest:
- We add a new sugar-only `Type` subclass that represents an alignment
attribute. Maybe we can model this as an `AttributedType` for the
`AlignedAttr`, or maybe we create a new kind of type node.
- We translate `AlignedAttr`s on typedef and type alias declarations into this
new kind of `Type` wrapping the declared type.
- We make `getTypeInfoImpl` special-case that type sugar node instead of
special-casing `TypedefType` sugar.
- We make sure that `TreeTransform` properly instantiates the new node, in
particular performing substitution within the argument.
Comment at: clang/lib/AST/ASTContext.cpp:2577-2583
+ case Type::SubstTemplateTypeParm: {
+const auto *SubType = cast(T);
+auto TI = getTypeInfo(SubType->getReplacementType().getTypePtr());
+if (MaybeGetAlignForTemplateAlias(TI, SubType))
+ TI.AlignRequirement = AlignRequirementKind::RequiredByTypedef;
+return TI;
+ }
Looking for a `SubstTemplateTypeParm` will mean that you don't handle cases
like:
```
template using AlignedChar [[gnu::aligned(N)]] = char;
```
Instead, it would be better to add the special treatment to
`TemplateSpecializationType`s for which `isTypeAlias` is `true`... but see my
other comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143533/new/
https://reviews.llvm.org/D143533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143533: [clang] Allow gnu::aligned attribute to work with templated type alias declarations
leonardchan created this revision.
leonardchan added reviewers: rsmith, rnk, echristo.
leonardchan added a project: clang.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.
leonardchan requested review of this revision.
Prior to this, the following would fail:
template
using NaturallyAligned [[gnu::aligned(sizeof(T))]] = T;
struct S { char x[2]; };
using AlignedS = NaturallyAligned;
static_assert(alignof(AlignedS) == 2); // alignof(AlignedS) == 1
This is because clang ignores attributes on type aliases when evaluating type
info and instead resolves to alignment of the replacement type `T`.
This patch attempts to check if the associated type declaration is an alias
that contains an attribute and return whatever the value is indicated by the
alignment expression.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143533
Files:
clang/lib/AST/ASTContext.cpp
clang/test/SemaTemplate/type-alias-aligned.cpp
Index: clang/test/SemaTemplate/type-alias-aligned.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/type-alias-aligned.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++17 -triple=x86_64-linux-gnu
+// expected-no-diagnostics
+
+template
+using Aligned8 [[gnu::aligned(8)]] = T;
+
+template
+using NaturallyAligned [[gnu::aligned(sizeof(T))]] = T;
+
+template
+using UnderAligned [[gnu::aligned(1)]] = T;
+
+template
+using NaturallyAligned2 [[gnu::aligned(S)]] = T;
+
+template
+using UnaryAligned [[gnu::aligned(+sizeof(T))]] = T;
+
+template
+using BinaryAligned [[gnu::aligned(sizeof(T) * 2)]] = T;
+
+struct S { unsigned x[2]; };
+static_assert(alignof(S) == 4);
+
+using Aligned8_S = Aligned8;
+static_assert(alignof(Aligned8_S) == 8);
+
+using NatAligned_S = NaturallyAligned;
+static_assert(alignof(NatAligned_S) == 8);
+
+using UnderAligned_S = UnderAligned;
+static_assert(alignof(UnderAligned_S) == 1);
+
+using NatAligned_S2 = NaturallyAligned2;
+static_assert(alignof(NatAligned_S2) == 8);
+
+using UnaryAligned_S = UnaryAligned;
+static_assert(alignof(UnaryAligned_S) == 8);
+
+using BinaryAligned_S = BinaryAligned;
+static_assert(alignof(BinaryAligned_S) == 16);
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -41,6 +41,7 @@
#include "clang/AST/RawCommentList.h"
#include "clang/AST/RecordLayout.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
#include "clang/AST/Type.h"
@@ -2026,6 +2027,138 @@
return TI;
}
+/// This class attempts to substitute the template parameter for a TypeAliasDecl
+/// with the replacement type for a SubstTemplateTypeParmType in order to create
+/// a new expression that is evaluatable (ie. not value-dependent). We need to
+/// create a new expression rather than replace componenets of an existing
+/// expression to recompute `dependence`s. Normally, this would be done via the
+/// TemplateDeclInstantiator, but that requires a reference to Sema which may
+/// not be available since the TypeInfo machinery only operates on the
+/// ASTContext.
+///
+/// NOTE: This class should be updated for various dependent expressions that
+/// can appear in an AlignedAttr's alignment expression. This can be done by
+/// adding more Visit methods.
+///
+class SubstTypeVisitor
+: public StmtVisitor {
+public:
+ SubstTypeVisitor(ASTContext , QualType ReplacementTy)
+ : Ctx(Ctx), ReplacementTy(ReplacementTy) {}
+
+ uint64_t TryEval(Expr *E) {
+Expr *NewE = Visit(E);
+if (!NewE || NewE->isValueDependent())
+ return 0;
+return NewE->EvaluateKnownConstInt(Ctx).getZExtValue() * Ctx.getCharWidth();
+ }
+
+ Expr *VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
+return new (Ctx) UnaryExprOrTypeTraitExpr(
+E->getKind(), Ctx.CreateTypeSourceInfo(ReplacementTy), E->getType(),
+E->getBeginLoc(), E->getEndLoc());
+ }
+
+ Expr *VisitDeclRefExpr(DeclRefExpr *E) {
+if (const auto *NTPD =
+dyn_cast(E->getFoundDecl())) {
+ if (Expr *DefaultArg = NTPD->getDefaultArgument())
+return Visit(DefaultArg);
+}
+return E;
+ }
+
+ Expr *VisitUnaryOperator(UnaryOperator *E) {
+Expr *SubExpr = Visit(E->getSubExpr());
+if (!SubExpr)
+ return nullptr;
+return UnaryOperator::Create(Ctx, SubExpr, E->getOpcode(), E->getType(),
+ E->getValueKind(), E->getObjectKind(),
+ E->getExprLoc(), E->canOverflow(),
+ E->getFPOptionsOverride());
+ }
+
+ Expr *VisitImplicitCastExpr(ImplicitCastExpr *E) {
+Expr *SubExpr = Visit(E->getSubExpr());
+if (!SubExpr)
+ return nullptr;
+return ImplicitCastExpr::Create(Ctx, E->getType(),
