[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:282
+  }
+  return "UNKNOWN_ENUM";
+}
----------------
I think these all these switch functions need to move the unknown case to default and use llvm_unreachable after the switch to avoid warnings on all compilers


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.h:520
+};
+llvm::StringRef getDecorationName(Decoration e);
+
----------------
Don't need llvm::


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:37
   if (Val) {
-    MIRBuilder.buildInstr(SPIRV::OpReturnValue).addUse(VRegs[0]);
-    return true;
+    const auto &STI = MIRBuilder.getMF().getSubtarget();
+    return MIRBuilder.buildInstr(SPIRV::OpReturnValue)
----------------
getSubtarget<SPIRVSubtarget>.I think it's worthwhile to just have this as a permanent member of SPIRVCallLowering


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:164
   // TODO: handle the case of multiple registers.
   assert(Info.OrigRet.Regs.size() < 2 && "Call returns multiple vregs");
 
----------------
This should still return false since multiple registers still can come from valid IR


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:172
+  if (Info.Callee.isGlobal()) {
+    const Function *CF = cast<const Function>(Info.Callee.getGlobal());
+    if (CF->isDeclaration()) {
----------------
This will still fail on constexpr casts and indirect calls. dyn_cast_or_null and return if you don't want to handle now


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:176
+      // to ensure VReg definition dependencies are valid across all MBBs.
+      MachineIRBuilder FirstBlockBuilder;
+      FirstBlockBuilder.setMF(MF);
----------------
You should not construct one off MachineIRBuilders. Re-use the one passed in and adjust the insert point. There's additional state (like CSEInfo) you're losing


================
Comment at: llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp:338
+SPIRVGlobalRegistry::getScalarOrVectorBitWidth(const SPIRVType *Type) const {
+  if (Type && Type->getOpcode() == SPIRV::OpTypeVector) {
+    auto EleTypeReg = Type->getOperand(1).getReg();
----------------
Why all the null checks on Type? I would expect all of these to require non-null inputs


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:204-207
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
+  MIRBuilder.setMBB(MBB);
+  MIRBuilder.setInstr(I);
----------------
iliya-diyachkov wrote:
> arsenm wrote:
> > You shouldn't have to construct a fresh new MIRBuilder for every selected instruction
> As I understand you suggest constructing MachineIRBuilder closer to its users, because not all instructions (for selection) are actually use it.
No, I mean MachineIRBuilder is a heavy class that should have state set up and maintained. It's not something to construct for a single use. The other selectors mostly just use BuildMI directly during selection.


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

https://reviews.llvm.org/D116464