[llvm] [SPIRV] Add a `SPIRVTypeInst` type with some guardrails (PR #179947)

[Marcos Maronas via llvm-commits]

Juan Manuel Martinez =?utf-8?q?Caamaño?=,
Juan Manuel Martinez =?utf-8?q?Caamaño?Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/179947 at github.com>


maarquitos14 wrote:

> 
> `SPIRVType` is `using SPIRVType = const MachineInstr;`.
> 
> The replacement may not be always exact (e.g. I haven't added the support for `DenseMap` so it cannot be used as a key). And changing all the uses of `SPIRVType` in one shot is a pretty big. And that is assuming that the assertion does not detect any problem.
> 
> We have to do it gradually.
> 
> I could also try to add the `[[deprecated]]` statement to `SPIRVType`. But we would have to cope with lots of warnings.

I totally get what you mean, and completely agree, I just don't know if having both at the same time is a good idea. From my point of view, they are trying to represent the same thing, and for newcomers it will be really difficult to understand the difference and choose between them. At the same time, if we don't start to try and fix it now, it'll only get worse. I'd like at least some commitment that there will be follow-ups to remove/replace SPIRVType. Is that reasonable? 

https://github.com/llvm/llvm-project/pull/179947