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

[Alex Bezzubikov via Phabricator via llvm-commits]

zuban32 added inline comments.


================
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());
 
----------------
iliya-diyachkov wrote:
> 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.
> @zuban32 no particular problem, except that this is the wrong place to call this interface

Why particularly? If you are aware of the exact issue this can cause pls share that

. 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.
> 

That's right, and that was supposed to be the aim of my subsequent patches.

> 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.
> 

>From what I see these targets are doing that mainly during or after selection.

> 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.

I think the problem that I was trying to solve is to avoid this 'manual' regclass setting (which BTW sometimes led to some verifier errors, I don't recall the exact usecase, I'll find it and post here) by leaving it all to the automated utilities based on the instruction description that already exists.

> 
> 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