[SYCL][ESIMD] Design alternatives for SYCLLowerIR/LowerESIMDVecArg.cpp

[kbobrovs]

This issue captures code review discussions for #2097.

This LLVM pass basically "unwraps" LLVM vectors wrapped into intel::gpu::simd objects when they are used as function formal
and actual parameters, plus inserts necessary bitcasts from vectors to intel::gpu::simd type (its LLVM type equivalent) before using the foramal parameters inside the function.
This is done to make IR compatible with the Intel GPU Vector Back-End and let it generate good code.

Doing such late type adjustment in LLVM IR based on formal parameter types is considered not optimal from the design standpoint by reviewers. Alternatives suggested:

1. (@andykaylor) Not rely on formal parameter types, but rely on types at the points of argument usages. See details below.
2. (@bader) Perform correct LLVM IR type generation in the FE
3. (@kbobrovs) Teach the Vector Back End to handle wrapped vectors and generate efficient code for them, thus skipping this type adjustment entirely

@andykaylor's explanation of 1):

Let me start by saying that there is a long-standing plan for pointers to become typeless in LLVM IR. Much work has been done toward this, and though much more needs to be done that will eventually be the state of the LLVM IR language. More to the point, already in LLVM IR the type associated with pointers has no semantic meaning. The existence of functions like PointerType::getElementType() may lead you to believe that the type has semantic meaning, but it does not. If we have transformations that rely on knowing the types of pointer arguments, that is a problem. Any information that can't be deduced from the way in which memory is accessed should not be relied upon.

Having said that, the above statement is good news for the change under review here, though not necessarily the way it's currently implemented. As I understand it, the effect of this patch is to replace the type %class._ZTSN2cm3gen4simdIiLi16EEE.cm::gen::simd with <16 x i32> throughout the Module. The patch works downward from function arguments and global variables, but that's essentially the end result, right? Do we need to do a similar replacement with alloca values?

Any correspondance between C++ types and similar types in the LLVM IR type system is nothing more than an implementation detail. There is no requirement for these types to be kept connected. The clang front end creates LLVM IR types as needed to perform a literal translation of the source code, which will describe the program semantics. The back end is then free to transform the IR in any way that maintains the same semantics. The front end probably does need to keep the AST-level type information until it generates IR, so we probably don't want to change the type there. With regard to pointer values once we generate LLVM IR, the following types are all exactly equivalent:

  %class._ZTSN2cm3gen4simdIiLi16EEE.cm::gen::simd*
 <16 x i32>*
 <8 x i64>*
 <64 x i8>*
 i8*

They are all pointers, and one of the fundamental guiding principles of LLVM IR is that memory does not have a type. So, what you're doing here is perfectly legal by LLVM IR rules.

As I said above, I do have concerns about the fact that we are relying on the types of pointers for certain transformations, but I'll put that aside for a moment and assume that perhaps the dependency isn't strictly as I've stated it and even if it is we can probably find another way to implement these transformations that removes the dependency.

The other thing I'd like to comment on, then, is the way this is implemented. You're starting from function arguments and global variables and then working down from there to update their uses. That will work fine for now, but when pointers become typeless it won't work. Global variables will, I think, continue to have a type associated with them in some way. Function arguments will not.

I think it might be better to structure the code now to examine the uses of arguments and global variables. Consider this function:

define void @f(%class._ZTS4simdIiLi16EE.simd* %x,
               %class._ZTS4simdIiLi16EE.simd* %y) {
entry:
  %x_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       %class._ZTS4simdIiLi16EE.simd* %x,
                                       i32 0, i32 0
  %x_data = load <16 x i32>, <16 x i32>* %x_data_ptr
  %y_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       %class._ZTS4simdIiLi16EE.simd* %y,
                                       i32 0, i32 0
  store <16 x i32> %x_data, <16 x i32>* %y_data_ptr
  ret void
}

With typeless pointers, that would look something like this:

Currently the element type of the pointer argument matches the source element type operand of the GEP instruction, but the source element type operand only exists because that won't always be true. The same is true of the type operand of the load and store instructions.

In fact, there is nothing stopping someone from transforming the first IR into this today:

define void @f(i8* %x,
               i8* %y) {
entry:
  %x_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       i8* %x,
                                       i32 0, i32 0
  %x_data = load <16 x i32>, i8* %x_data_ptr
  %y_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       i8* %y,
                                       i32 0, i32 0
  store <16 x i32> %x_data, i8* %y_data_ptr
  ret void
}

or even this:

define void @f(i8* %xA,
               i8* %yA) {
entry:
  %x = bitcast i8* &xA to %class._ZTS4simdIiLi16EE.simd*
  %x_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       %class._ZTS4simdIiLi16EE.simd* %x,
                                       i32 0, i32 0
  %x_data = load <16 x i32>, <16 x i32>* %x_data_ptr
  %y = bitcast i8* &yA to %class._ZTS4simdIiLi16EE.simd*
  %y_data_ptr = getelementptr inbounds %class._ZTS4simdIiLi16EE.simd,
                                       %class._ZTS4simdIiLi16EE.simd* %y,
                                       i32 0, i32 0
  store <16 x i32> %x_data, <16 x i32>* %y_data_ptr
  ret void
}

My point is that the way the arguments are used gives you semantic information about what they point to. The pointer types do not.

Tagging @pratikashar

[pratikashar]

• @cmc-rep

[KornevNikita]

Hi! There have been no updates for at least the last 60 days, though the ticket has assignee(s).

@pratikashar, could I ask you to take one of the following actions? :)

• Please provide an update if you have any (or just a small comment if you don't have any yet).
• OR mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it.
• OR close the issue if it has been resolved.
• OR take any other suitable action.

Thanks!

[pratikashar]

@KornevNikita - I no longer work on ESIMD compiler. So it would be nice if someone active in this project could take over this task.

OR mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it.

Has ESIMD compiler been updated to use llvm version that uses opaque pointers?

I think the next step should be to take llvm IR with opaque pointer support and update this pass based on suggestion from @andykaylor.

[sarnex]

With the move to opaque pointers this pass has been removed, so closing the issue.