[Backport to 17] Fix incorrect translation of calls to a builtin that returns a structure (#2722) #3030
Conversation
… returns a structure
… returns a structure
| @@ -2269,12 +2270,18 @@ bool postProcessBuiltinReturningStruct(Function *F) { | |||
| NewF->setCallingConv(F->getCallingConv()); | |||
| auto Args = getArguments(CI); | |||
| Args.insert(Args.begin(), A); | |||
| CallInst *NewCI = Builder.CreateCall(NewF, Args, CI->getName()); | |||
| CallInst *NewCI = Builder.CreateCall( | |||
| NewF, Args, NewF->getReturnType()->isVoidTy() ? "" : CI->getName()); | |||
There was a problem hiding this comment.
Why do we change Name argument to CreateCall? Neither the llvm-17 branch, nor your fix to the main branch contains this change
There was a problem hiding this comment.
I've hit an error on the 18th LLVM version with test/builtin_returns_struct.spvasm -- given a newly inserted call spir_func void @_Z17__spirv_IAddCarryii(ptr sret(%structtype) %[[#Var:]] the code tried to use a non-empty name for the instruction and LLVM failed with an error. This check prevents this fail.
There was a problem hiding this comment.
More specifically, the error was due to an attempt to assign name to a void-typed instruction
There was a problem hiding this comment.
Ok, understandable. Although, I don't get why we don't face this issue on the main branch or even on the llvm-19. Do you have any thoughts/observations?
There was a problem hiding this comment.
It's a very good question actually. My observation is that Builder.CreateCall(NewF, Args, CI->getName()) has empty CI->getName() due to some differences in processing between branches. I'd say that we should port this backport finding and harden the main branch with this check NewF->getReturnType()->isVoidTy() ? "" : CI->getName() as well.
There was a problem hiding this comment.
I've added the same check for a void-typed instruction into the "Backport to 19" patch.
There was a problem hiding this comment.
@vmaksimo do you have any followup to the Name argument to CreateCall please?
There was a problem hiding this comment.
@MrSidims actually I approved before this check was added.
That's still strange, but let's proceed with it.
VyacheslavLevytskyy
changed the title
VyacheslavLevytskyy
merged commit eccbadc
into
KhronosGroup:llvm_release_170
This PR fixes the issue with Khronos Translator incorrectly translating calls to builtins that returns a structure and generating incorrect extractvalue applied to a pointer. The PR is to fix the issue by preserving equivalence of newly inserted values with prior values of function return's type.