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

[Chris Bieneman via Phabricator via cfe-commits]

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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129883