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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added a comment.
This revision is now accepted and ready to land.

LGTM with nits



================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:283
+  default:
+    return "UNKNOWN_ENUM";
+  }
----------------
the llvm_unreachable is a bit pointless without a break here


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:93-95
+      if (Arg.hasAttribute(Attribute::Alignment)) {
+        auto Alignment = static_cast<unsigned>(
+            Arg.getAttribute(Attribute::Alignment).getValueAsInt());
----------------
getParamAlignment would be nicer


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:810
+             .addUse(GR.getSPIRVTypeID(SpvI32Ty));
+  else
+    MI = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpConstantI))
----------------
Braces


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:995
+  MachineBasicBlock &MBB = *I.getParent();
+  if (PrevI != nullptr && PrevI->getOpcode() == TargetOpcode::G_BRCOND)
+    return BuildMI(MBB, I, I.getDebugLoc(), TII.get(SPIRV::OpBranchConditional))
----------------
Braces. Line wrapping is a bit weird looking


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:1040
+  const unsigned NumOps = I.getNumOperands();
+  assert((NumOps % 2 == 1) && "Require odd number of operands for G_PHI");
+  for (unsigned i = 1; i < NumOps; i += 2) {
----------------
Not much point asserting on things the verifier checks


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:204-207
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
+  MIRBuilder.setMBB(MBB);
+  MIRBuilder.setInstr(I);
----------------
iliya-diyachkov wrote:
> iliya-diyachkov wrote:
> > arsenm wrote:
> > > 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.
> > Thanks, now I see that only AMDGPU and Mips targets sometimes use MachineIRBuilder. I'll  change it in SPIRV.
> I have left only one (relatively rare) case of using MachineIRBuilder in SPIRVInstructionSelector. I suppose this is acceptable since AMDGPU and Mips do it in a similar way.
I've been meaning to fix those; I wouldn't use that as a good example.


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

https://reviews.llvm.org/D116464