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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov marked 3 inline comments as done.
iliya-diyachkov added inline comments.


================
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)
----------------
arsenm wrote:
> getSubtarget<SPIRVSubtarget>.I think it's worthwhile to just have this as a permanent member of SPIRVCallLowering
Well, it looks reasonable.


================
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()) {
----------------
arsenm wrote:
> This will still fail on constexpr casts and indirect calls. dyn_cast_or_null and return if you don't want to handle now
Thanks, it'll be fixed.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:176
+      // to ensure VReg definition dependencies are valid across all MBBs.
+      MachineIRBuilder FirstBlockBuilder;
+      FirstBlockBuilder.setMF(MF);
----------------
arsenm wrote:
> 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
Sure, this is bad code.


================
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();
----------------
arsenm wrote:
> Why all the null checks on Type? I would expect all of these to require non-null inputs
Thanks, will fix.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp:204-207
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
+  MIRBuilder.setMBB(MBB);
+  MIRBuilder.setInstr(I);
----------------
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.


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

https://reviews.llvm.org/D116464