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

[Chris Bieneman via Phabricator via cfe-commits]

beanz added inline comments.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:46
+    0, // ptr64
+    // HLSL address space values for this map are dummy and they can't be used
+    0, // hlsl_cbuffer
----------------
I don’t think you need this comment. Most of those adds space values aren’t applicable to these targets, that’s why they are all 0.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:185
+  const unsigned TBufferAddressSpace =
+      ASMap[(unsigned)clang::LangAS::hlsl_tbuffer];
+
----------------
Why look both of these values up when you’re only going to use one?


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:189
+    layoutBuffer(Buf, DL);
+    auto AddressSapce =
+        Buf.IsCBuffer ? CBufferAddressSpace : TBufferAddressSpace;
----------------
nit: Don’t use `auto` in places where the type isn’t obvious from the expression.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:204
+  } else {
+    Reg = UINT_MAX;
+    Space = 0;
----------------
nit: Rather than using int max as a sentinel here, why not use an llvm::Optional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130131