Emit OpMemberDecorate for member 0 annotations #1646
Conversation
LLVMToSPIRVBase::transIntrinsicInst translates ptr.annotation intrinsics marking struct members by looking for struct GEPs. However, a GEPs is not necessary for the first member, and so in some cases when the first member is annotated SPIRVWriter has been emitting OpDecorate for these instead which leads to a crash in some cases on reverse translation. This change enables emitting OpMemberDecorate for the first member even if there isn't a GEP and the pointer value is based on a struct alloca instead.
|
This is another approach to the problem in #1635 which seems to be a more proper fix |
| @@ -3745,15 +3745,38 @@ SPIRVValue *LLVMToSPIRVBase::transIntrinsicInst(IntrinsicInst *II, | |||
| AnnotationDecorations Decorations = | |||
| tryParseAnnotationString(BM, AnnotationString); | |||
|
|
|||
| // Translate FPGARegIntel annotations to OpFPGARegINTEL. | |||
There was a problem hiding this comment.
This change seems to be unrelated to the issue being addressed. Can you please comment? Thanks
There was a problem hiding this comment.
This was something I discovered when running the tests: there are a few cases where this FPGARegIntel annotation is used on struct values, and my change was incorrectly trying to translate those as OpMemberDecorate instead. It seems like in cases where we have an FPGARegIntel annotation we want to translate it to OpFPGARegINTEL regardless of whether we find a struct type, so I thought the cleanest way of doing that would be moving this check up here.
@vmaksimo, from the git log it looks like you originally added this code; is putting it here correct?
There was a problem hiding this comment.
@dwoodwor-intel Oh, that has been some long time ago, and I missed a big part of changes related to this feature transformation :)
But I do think that now it's a correct place - it looks logical and has not failed any tests.
| addAnnotationDecorations(DecSubj, Decorations.MemoryAccessesVec); | ||
| II->replaceAllUsesWith(II->getOperand(0)); | ||
| } | ||
| // Memory accesses to a standalone pointer variable |
There was a problem hiding this comment.
Can you please specify if the scope of this change goes beyond addressing the first member of the struct variable? This change also seems unrelated. Thanks
There was a problem hiding this comment.
I've just removed the OpFPGARegINTEL code from here; the rest of the changes in this part are just formatting
I ran spirv-val on this test and did find an error: I also tried without this change on the main branch and I see the same error there too. It seems to be a preexisting issue unrelated to this change. |
|
llvm-bolt is a known issue reported in llvm/llvm-project#58317 and several other places. Merging. |
LLVMToSPIRVBase::transIntrinsicInst translates ptr.annotation intrinsics marking struct members by looking for struct GEPs. However, a GEPs is not necessary for the first member, and so in some cases when the first member is annotated SPIRVWriter has been emitting OpDecorate for these instead which leads to a crash in some cases on reverse translation. This change enables emitting OpMemberDecorate for the first member even if there isn't a GEP and the pointer value is based on a struct alloca instead.
This change also moves the handling of FPGARegIntel annotations so that they're translated to OpFPGARegINTEL before attempting to translate struct member annotations. This ensures that they don't get incorrectly translated with OpMemberDecorate.