[PATCH] D116464: [SPIRV 5/6] Add LegalizerInfo, InstructionSelector and utilities

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:1406
+    return "Acquire";
+  } else if ((MemorySemantics::Acquire != 0) &&
+             (e & MemorySemantics::Acquire)) {
----------------
No else after return


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h:22
+
+namespace Capability {
+enum Capability : uint32_t {
----------------
SPIRV namespace?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:205
+  auto M = MIRBuilder.getMF().getFunction().getParent();
+  Function *Callee = M->getFunction(FuncName);
+  Register FuncVReg;
----------------
iliya-diyachkov wrote:
> arsenm wrote:
> > You should absolutely not be checking the module or relying on the callee being a named function. You should only be inspecting the provided CallLoweringInfo for callee information. This will fail on indirect calls or if the callee is a constexpr bitcast
> Thanks, it will be fixed.
the static_cast to function is also no good since this could be a constexpr cast


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:150
+  if (GV)
+    GVar = dyn_cast<const GlobalVariable>(GV);
+  else {
----------------
Use cast here and remove the assert below? 


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:159
+      GVar = new GlobalVariable(
+          *const_cast<llvm::Module *>(Module), const_cast<Type *>(Ty), false,
+          GlobalValue::ExternalLinkage, nullptr, Twine(Name));
----------------
no const_casts?


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

https://reviews.llvm.org/D116464