[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:61
+
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;
----------------
python3kgae wrote:
> Anastasia wrote:
> > I don't think I understand the intent of this function along with `CGHLSLRuntime::addConstant` that populates this collection.
> It is translating
> 
> ```
>  cbuffer A {
>    float a;
>    float b;
> }
> float foo() {
>   return a + b;
> }
> ```
> into
> 
> ```
> struct cb.A.Ty {
>   float a;
>   float b;
> };
> 
> cbuffer A {
>   cb.A.Ty CBA;
> }
> float foo() {
>   return CBA.a + CBA.b;
> }
> ```
> 
> Both a and b are in the global scope and will get a GlobalVariable in clang codeGen.
> By doing the translation, we can ensure each buffer map to one GlobalVariable and save cbuffer layout in the value type of that GlobalVariable.
Ok, I see, so if we are to translate it to C it would be something similar to:


```
struct A {
   float a;
   float b;
} cbuffer_A __attribute__((address_space(256)));

float foo() {
  return cbuffer_A.a + cbuffer_A.b;
}
```
Maybe you can add some comments to explain the intent of this code at a higher level... not sure if the generation can reuse or be made a bit close to the regular C structs + address spaces...



================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:119
+void CGHLSLRuntime::addConstant(VarDecl *D, Buffer &CB) {
+  if (D->getStorageClass() == SC_Static) {
+    // For static inside cbuffer, take as global static.
----------------
btw is this covered in the test?


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:152
+      // as it only refers to globally scoped declarations.
+      CGM.EmitTopLevelDecl(it);
+    } else if (NamespaceDecl *ND = dyn_cast<NamespaceDecl>(it)) {
----------------
Ok I think you don't have this in the tests too and the one below?



================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:183
+  const unsigned CBufferAddressSpace =
+      ASMap[(unsigned)clang::LangAS::hlsl_cbuffer];
+  const unsigned TBufferAddressSpace =
----------------
I think it's better to use `getTargetAddressSpace` from `ASTContext`.


================
Comment at: clang/test/CodeGenHLSL/nest_cbuf.hlsl:8
+  // CHECK: @[[TB:.+]] = external addrspace(5) constant { float, i32, double }
+  tbuffer A : register(t2, space1) {
+    float c;
----------------
is this generated as nested structs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131