[PATCH] D17497: Support arbitrary address space for intrinsics

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:

  1. I like the cleaner naming scheme.
  2. I’m not sure the additional complexity is worth it. (Not specific to the particular implementation proposed here.)
  3. 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.

What do others think?

Philip

D17497.48653.patch (13.1 KB)

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.

David

+1

The prettiness of intrinsic mangling is irrelevant, especially since this will all change with typeless pointers anyway.

-Matt

Typeless pointers will still have an address space, right?

Yes

One more possible solution that I considered is allowing forward reference in types.

def int_xxx_yyy : Intrinsic<[LLVMVectorElt<1>], [llvm_anyvector_ty], [IntrNoMem]>;

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.

- Elena

Elena,

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.

Philip

Per my previous email, I have just signed off on Artur’s original patch.

Philip

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.

It’s not verified by intrinsic overloading mechanism, but there is an explicit check about masked load and store argument types.

Artur