[PATCH] D129883: [HLSL] Support cbuffer/tbuffer for hlsl.

[Xiang Li via Phabricator via cfe-commits]

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