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

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:61
+
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;
----------------
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.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:62
+GlobalVariable *replaceCBuffer(CGHLSLRuntime::CBuffer &CB) {
+  const unsigned CBufferAddressSpace = 4;
+  const unsigned TBufferAddressSpace = 5;
----------------
Anastasia wrote:
> It might be better to avoid using hard-coded constants. Are you adding new entires in clang's `AddressSpace` enum to represent different logical memory segment of the language?
Thanks for pointing it out. I'll add the new address space to LangAS.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:142
+      if (!Inner) {
+        DiagnosticsEngine &Diags = CGM.getDiags();
+        unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
----------------
Anastasia wrote:
> Is this case covered by the test?
Nice catch.
I'll add a test for this.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:39
+    llvm::StringRef Name;
+    bool IsCBuffer;
+    unsigned Reg;
----------------
Anastasia wrote:
> what does this field represent?
The field is for marking the CBuffer as a cbuffer or tbuffer.
The name is confusing.
How about adding an enum BufferKind { CBuffer, TBuffer };?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131