This probably needs broader discussion. We have an existing naming mechanism for polymorphic intrinsics; Elena is proposing a new one to avoid making the names for various load/store intrinsics particularly ugly.
My personal take:
I like the cleaner naming scheme.
I’m not sure the additional complexity is worth it. (Not specific to the particular implementation proposed here.)
I have no strong preference other than that the @llvm.masked_load (and friends) intrinsics support alternate address spaces in some form in the near future.
My gut feeling is that it’s not worth it. When we move from typed to untyped pointers, we’re going to change the mangling from something like p200i8 to just p200, which is already quite a bit cleaner, and actually looks cleaner to me than the version proposed in this patch.
For example, you want to add a vector operation that returns scalar:
In the current implementation it will look like
i32 @llvm.vec.i32.v8i32(<8 x i32>%x)
If you allow forward reference, it will look like
i32 @llvm.vec.v8i32(<8 x i32>%x).
The same about masked intrinsics. @llvm.masked.load.p0v8i32.v8i32 - you have a long name with duplicated info just due to the limitation in the current infrastructure.
I’d like to propose that we move forward with Artur’s original patch and separate the discussion of how we might change our intrinsic naming scheme. Artur’s patch is addressing a correctness problem; that has to overrule stylistic concerns. We are seeing failures in our nightly tests due to this issue on an ongoing basis, and I’d really like to get the correctness issue resolved in the immediate future.
I am more than happy to continue the discussion about better naming schemes - in particular, I like you’re idea of potentially allowing forward references - but I strongly feel we need to decouple it from a bug fix for a correctness issue.
I see your point. I’ll try to implement forward reference in intrinsics pattern. Otherwise verifier does not check correlation between pointer and data types.