python3kgae added inline comments.
================
Comment at: clang/lib/Parse/ParseHLSL.cpp:79-80
+ ParsedAttributes Attrs(AttrFactory);
+ MaybeParseCXX11Attributes(Attrs);
+ MaybeParseMicrosoftAttributes(Attrs);
+
----------------
beanz wrote:
> 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.
Removed attribute parsing.
Will add it back when real HLSL attributes like packoffset are supported.
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
