[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/SPIRVInstructionSelector.cpp:204-207
+  MachineIRBuilder MIRBuilder;
+  MIRBuilder.setMF(MF);
+  MIRBuilder.setMBB(MBB);
+  MIRBuilder.setInstr(I);
----------------
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.


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

https://reviews.llvm.org/D116464