[NFCI] Use TypedPointerType in more places. #1683
Conversation
One of the reasons behind this change is to make code more easily deal with the
future prospect of opaque types, so that helper methods (like adjusting image
types) can handle both pointer type and opaque type representations simply by
querying if the input type is a TypedPointerType or an OpaqueType [name for the
latter still pending].
The set of changes are:
* OCLTypeToSPIRV now uses TypedPointerType internally
* adaptSPIRVImageType and getSPIRVStructTypeByChangeBaseTypeName are collapsed
into one method (adjustImageType) that works with TypedPointerTypes.
* A few is*StructType methods have been reverted back to is*Type methods, taking
a TypedPointerType parameter instead.
* BuiltinCallHelper::addSPIRVCall{Pair} allows for the creation of SPIR-V calls
that can use TypedPointerType or actual type for parameters and return value.
* BuiltinCallHelper::getCallValue{Type} is a simple helper that hides many of
the uses of getParameterTypes.
* The Type* parameter of the callback in BuiltinCallMutator::mapArg now provides
a TypedPointerType or the actual type, instead of the pointer element type.
* BuiltinCallMutator::ValueTypePair similarly takes a TypedPointerType or the
actual type.
* getParameterTypes (when passed a SmallVector<Type *>) does a similar thing.
(The SmallVector<TypedPointerType *> variant has been removed in favor of only
using the other one.)
Note that the last few changes do change the semantics of function parameters
without changing the function name or signature.
| std::move(NameMapFn)); | ||
| if (!DidDemangle) { | ||
| // TODO: PipeBlocking.ll causes demangling failures. | ||
| // assert(isNonMangledOCLBuiltin(CI->getCalledFunction()->getName()) && |
There was a problem hiding this comment.
It does look like that if a function name has .1 in the end (for example
declare spir_func void @_Z30__spirv_WritePipeBlockingINTELPU3AS115__spirv_Pipe__1PU3AS4iii.1(%opencl.pipe_wo_t addrspace(1)* %0, i9 addrspace(4)* %1, i32 %2, i32 %3)
RootNode will be nullptr
auto *RootNode = dyn_cast_or_null<FunctionEncoding>(Demangler.parse());
unsure if it's expected
There was a problem hiding this comment.
If the .1 is added, then the function name isn't a valid mangled name which is... probably problematic, but that requires some more investigation.
There was a problem hiding this comment.
Agree. May be it ends up a problem with pipes translation.
| MangledName.end()); | ||
| // We expect to see only function name encodings here. If it's not a function | ||
| // name encoding, bail out. | ||
| auto *RootNode = dyn_cast_or_null<FunctionEncoding>(Demangler.parse()); |
There was a problem hiding this comment.
So there is from where we get false to trigger the assert.
Co-authored-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
|
Friendly review ping? |
Sorry for the delay, I was already working through it, but as it has a lot of scattered changes I'll need some more time. |
3 participants
One of the reasons behind this change is to make code more easily deal with the future prospect of opaque types, so that helper methods (like adjusting image types) can handle both pointer type and opaque type representations simply by querying if the input type is a TypedPointerType or an OpaqueType [name for the latter still pending].
The set of changes are:
Note that the last few changes do change the semantics of function parameters without changing the function name or signature.