[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;
----------------
I don't think I understand the intent of this function along with `CGHLSLRuntime::addConstant` that populates this collection.


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


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:142
+      if (!Inner) {
+        DiagnosticsEngine &Diags = CGM.getDiags();
+        unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
----------------
Is this case covered by the test?


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:39
+    llvm::StringRef Name;
+    bool IsCBuffer;
+    unsigned Reg;
----------------
what does this field represent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131