[PATCH] D153099: [SPIR-V] Begin removing explicit setRegClass calls

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.
Herald added a subscriber: pmatos.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp:321-322
+  const auto &ST = GR->CurMF->getSubtarget();
+  constrainSelectedInstRegOperands(*B.getInstr(), *ST.getInstrInfo(),
+                                   *ST.getRegisterInfo(), *ST.getRegBankInfo());
 
----------------
zuban32 wrote:
> iliya-diyachkov wrote:
> > `constrainSelectedInstRegOperands` should be called from InstructionSelector only. We also have it in methods of SPIRVGlobalRegistry but these methods are called from InstructionSelector.
> This is the whole point of the patch - these instructions are built before selection, but we need to have them constrained in order to support selection of instruction that depend on them (like G_ADD adding two OpFunctionParameter's). Using setRegClass explicitly is wrong since these classes are already described by the instruction description in tablegen. I remember @arsenm pointed out the problem a while ago.
> 
> @iliya-diyachkov  do you see any particular issue with calling these funcs from here? I see there're a couple of non-selector uses in AMDGPU RegBankSelect and MIPS legalizer. Anyway, I don't see any viable alternative.
@zuban32 no particular problem, except that this is the wrong place to call this interface. As you write, you are going to continue removing setRegClass in builtins-related code, so you will need to insert much more calls to `constrainSelectedInstRegOperands` outside of InstructionSelector.

Am I right that [[ https://reviews.llvm.org/D153101| D153101]] patch (with a small fix) can be applied without this one? In my sandbox, it passes all lit tests. If so, it would be nice to test it on OpenCL CTS.

I understand that using setRegClass is logically excessive (and may look unsightly) since we have the info in the instruction description, however (as I see it) other targets also use setRegClass in this way.

On other hand, as we discussed earlier, we are going to exclude "global" instructions from the function level. Many instructions (which require setRegClass) are "global", so they will go away from IR with no need to call constrainSelectedInstRegOperands outside of InstructionSelector. Generation of others (mainly in SPIRVBuiltins.cpp) can probably be moved to InstructionSelector. After this, we will have a few instructions that require setting of reg class before InstructionSelector (like in AMDGPU/MIPS), which is acceptable.

So, I propose either to postpone this patch and commit other two patches (and continue the plan described before), or to mark all inserted `constrainSelectedInstRegOperands` calls with "FIXME" comment with the obligation to remove them when we find a better solution, and try not to spread such use of `constrainSelectedInstRegOperands` in the next patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153099