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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/MCTargetDesc/SPIRVBaseInfo.cpp:283
+  default:
+    return "UNKNOWN_ENUM";
+  }
----------------
arsenm wrote:
> the llvm_unreachable is a bit pointless without a break here
I see, will change to break.


================
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());
----------------
arsenm wrote:
> getParamAlignment would be nicer
It'll be corrected.


================
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) {
----------------
arsenm wrote:
> Not much point asserting on things the verifier checks
Will remove.


================
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:
> > 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.
Ok, if you don't mind I'll add FIXME for this last case for now and fix it as soon as possible. This fix may require an additional extension in SPIRVGlobalRegistry, in which case I would commit it after this initial series of patches.


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

https://reviews.llvm.org/D116464