Skip to content

Fix reverse translation of ptr.annotation for multiple UserSemantic decorations#1723

Merged
MrSidims merged 11 commits intoKhronosGroup:mainfrom
vmaksimo:wip_fix_reverse_ptr_ann
Jan 26, 2023
Merged

Fix reverse translation of ptr.annotation for multiple UserSemantic decorations#1723
MrSidims merged 11 commits intoKhronosGroup:mainfrom
vmaksimo:wip_fix_reverse_ptr_ann

Conversation

@vmaksimo
Copy link
Contributor

@vmaksimo vmaksimo commented Nov 17, 2022

It was found that the decorated variables (results of ptr.annotation intrinsic) are not used in further instructions unlike the input source. The issue is present for the reverse translation of multiple UserSemantic decorations.

This change fixes the reverse translation of multiple ptr.annotation intrinsics on the same class field. With this, the result of intrinsic is not lost and actually reused in further intrinsics and instructions.

@vmaksimo vmaksimo force-pushed the wip_fix_reverse_ptr_ann branch from 57936c9 to f482fca Compare December 7, 2022 17:16
@vmaksimo vmaksimo changed the title [WIP] fix reverse ptr annotations translation Fix reverse translation of ptr.annotation Dec 7, 2022
@vmaksimo vmaksimo marked this pull request as ready for review December 7, 2022 17:40
@vmaksimo
Copy link
Contributor Author

vmaksimo commented Dec 7, 2022

@MrSidims @asudarsa please take a look. Thanks!

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM
@tiwaria1 may I ask you to check if this patch causes regressions on your side?

@vmaksimo vmaksimo requested a review from MrSidims December 12, 2022 10:29
@vmaksimo vmaksimo changed the title Fix reverse translation of ptr.annotation Fix reverse translation of ptr.annotation for multiple UserSemantic decorations Dec 16, 2022
@MrSidims
Copy link
Contributor

Restarting CI with a new base

@MrSidims MrSidims closed this Dec 21, 2022
@MrSidims MrSidims reopened this Dec 21, 2022
@vmaksimo
Copy link
Contributor Author

The test failure is not related to this patch. I found that the translation fails on this line call ptr @llvm.ptr.annotation.p0.p0(ptr %16, ptr %17, ptr @.str.1, i32 12, ptr null), because we expect the string constant instead of bitcast as a second argument. The IR has changed after type scavenger making input incorrect.

// input
%11 = getelementptr inbounds %class.A, ptr %2, i32 0, i32 1 
%12 = call ptr @llvm.ptr.annotation.p0.p0(ptr %11, ptr @.str.2, ptr @.str.1, i32 12, ptr null) 
%13 = call ptr @llvm.ptr.annotation.p0.p0(ptr %12, ptr @.str.3, ptr @.str.1, i32 12, ptr null)

// IR after type scavenger magic
%15 = getelementptr inbounds %class.A, ptr %2, i32 0, i32 1 
%16 = bitcast ptr %15 to ptr 
%17 = bitcast ptr @.str.2 to ptr 
%18 = call ptr @llvm.ptr.annotation.p0.p0(ptr %16, ptr %17, ptr @.str.1, i32 12, ptr null) 
%19 = bitcast ptr @.str.3 to ptr 
%20 = call ptr @llvm.ptr.annotation.p0.p0(ptr %18, ptr %19, ptr @.str.1, i32 12, ptr null)

Found that #1775 fixes this issue. I believe we should merge it before this one.

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
But would prefer if someone else from Intel took a look.

@vmaksimo
Copy link
Contributor Author

@dwoodwor-intel @sarnex @asudarsa please take a look. Thanks!

@sarnex
Copy link
Contributor

sarnex commented Jan 12, 2023

I don't fully understand this but if nobody else responds ping me and we can go over it

@MrSidims
Copy link
Contributor

MrSidims commented Jan 17, 2023

I don't fully understand this but if nobody else responds ping me and we can go over it

That's actually the point - to share some expertise and have someone with a fresh look at annotations translations, since, it does look like the initial 'design' was not the best and two authors of this 'design' are author and reviewer of this patch :D

@vmaksimo
Copy link
Contributor Author

@MrSidims can we still merge this one? I believe no one is concerned about the change :)

@MrSidims MrSidims merged commit 308daff into KhronosGroup:main Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants