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

Mon Oct 17 02:27:19 PDT 2022

nhaehnle added a comment.

In D135202#3860361 <https://reviews.llvm.org/D135202#3860361>, @nikic wrote:

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

Yeah, that's a fair point.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202