beanz added inline comments.
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ ParsedAttributes Attrs(AttrFactory);
+ MaybeParseCXX11Attributes(Attrs);
+ MaybeParseMicrosoftAttributes(Attrs);
+
----------------
aaron.ballman wrote:
> beanz wrote:
> > aaron.ballman wrote:
> > > beanz wrote:
> > > > python3kgae wrote:
> > > > > aaron.ballman wrote:
> > > > > > Just double-checking, but this allows `[[]]` style attributes as
> > > > > > well as `[]` style attributes, but not `__attribute__` or
> > > > > > `__declspec` style attributes, is that intended?
> > > > > That is what dxc currently support.
> > > > > It may change in the future. But for now, only [[]] and [] are
> > > > > supported.
> > > > Well... worth noting, HLSL doesn't actually support C++11 attributes,
> > > > but that is almost certainly going to change in the near future, so we
> > > > might as well support them from the start in Clang.
> > > Ah, that is good to know @beanz -- we should think carefully about this
> > > situation because there are some tradeoffs to consider.
> > >
> > > 1) It's pretty weird to support half of the Microsoft attribute syntax
> > > (and the half we barely have any attribute support for, at that). Is
> > > there a reason to not support `__declspec` as well? (For example, are
> > > there concerns about things like using those attributes to do dllexport
> > > or specify a COMDAT section, etc?) In fact, if we're going to support
> > > vendor attributes like `[[clang::overloadable]]`, it's a bit weird that
> > > we then prohibit the same attribute from being spelled
> > > `__attribute__((overloadable))`, is there a particular reason to not
> > > extend to all attributes?
> > > 2) Supporting `[]` style attributes means that attribute order is
> > > important. We cannot use `MaybeParseAttributes()` to parse attribute
> > > specifiers in any order because `[]` causes ambiguities under some
> > > circumstances. So you're stuck with the order you have -- `[[]]`
> > > attributes first, `[]` attributes second. Is that ordering reasonable?
> > >
> > > And for the patch itself -- there are no test cases demonstrating what
> > > happens when using attributes on things within one of these buffers. I
> > > expect many things to be quite reasonable, like using `[[deprecated]]`,
> > > but are the attributes which impact codegen reasonable as well? (Like
> > > naked functions, returns twice, disable tail calls, etc)
> > @aaron.ballman I think those are all good questions. Generally HLSL has
> > used Microsoft attribute syntax, and I've started extending the Clang
> > support to be more robust, but still have more work to do.
> >
> > More on this patch, I want to take a step back.
> >
> > I think @python3kgae copied this code from DXC, but I don't think it is
> > ever used. I don't think we have any attributes in the language that are
> > valid with cbuffer or tbuffer subjects. We certainly don't have any
> > attributes implemented in clang that would be valid on these targets.
> >
> > That makes me think we should remove since it should be dead and
> > unreachable and untestable code.
> >
> > Since these HLSL buffer decls are an older (although frequently used) HLSL
> > feature, I think our general preference is to not extend new feature
> > support to them, and instead to encourage users to use the newer buffer
> > types.
> >
> > Does that sound reasonable?
> > We certainly don't have any attributes implemented in clang that would be
> > valid on these targets.
>
> Despite knowing nothing about HLSL, I feel like pushing back a little bit
> here: deprecated, nodiscard, maybe_unused, and many others seem like they'd
> not only be valid on the target but perhaps useful to users.
>
> > Does that sound reasonable?
>
> I'm totally fine with that approach; we can debate attributes later. :-)
> Despite knowing nothing about HLSL, I feel like pushing back a little bit
> here: deprecated, nodiscard, maybe_unused, and many others seem like they'd
> not only be valid on the target but perhaps useful to users.
Okay... you got me here. I hadn't considered `deprecated` but can see a use for
it. I don't think the other two apply, but I'll concede there may be more
general clang attributes that do have uses.
If we can postpone this discussion though I think we can do some background and
get a better feeling for what attributes we should and shouldn't support, and
maybe consider the syntax a bit carefully too.
If I'm reading this correctly the DXC-supported syntax is:
```
cbuffer A { ... } [some_attribute]
```
(note: DXC doesn't really support CXX11 attributes, just the MS syntax)
If this syntax is really unreachable in DXC (which I believe it is), it might
be better to shift the syntax to be more like C++ class and struct attributes:
```
[[some_attribute]]
cbuffer A {...}
```
I think that would be more familiar and understandable to users, especially as
buffer declarations are sometimes hundreds of lines long.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129883/new/
https://reviews.llvm.org/D129883
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
