[PATCH] D135202: [IR] Add an opaque type to LLVM.

Sat Oct 15 02:37:26 PDT 2022

nikic added a comment.

In D135202#3843673 <https://reviews.llvm.org/D135202#3843673>, @jcranmer-intel wrote:

> Sorry for the delay in responding to comments, I've been trying to get the subsequent changes that use for SPIR-V polished.
>
> In D135202#3836252 <https://reviews.llvm.org/D135202#3836252>, @nikic wrote:
>
>> The implementation looks reasonable. In terms of design, I basically have the same concern as everyone else: Hardcoding the layout to `ptr` seems pretty dubious. The suggestion to specify the layout type as a parameter seems pretty sensible.
>
> As pointed out, specifying an underlying type isn't necessarily feasible.

I couldn't find where this has been pointed out, could you please link me to it? I don't really get why it wouldn't be feasible, given that your current proposal effectively already does it, just with a hardcoded `ptr` type.

> But I could add optional type properties that would include size and alignment to start with (and have those default to `sizeof(ptr)` and size, respectively)--how does that sound?

That would be fine as well, though probably harder to implement.

In D135202#3855371 <https://reviews.llvm.org/D135202#3855371>, @nhaehnle wrote:

> Here's a random thought: what if the types are parameterized using metadata instead of ints?

I was originally going to suggest to parameterize them by `Constant`s. But then I realized that this would add a cyclic dependency between values and types. Currently values only use types, but not the other way around. It's probably possible to support this, but it seems like a very large complication that's unlikely to be justified. The same concern holds for metadata (probably to an even larger degree).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202