Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Visitor to use CallInst #115

Merged

Conversation

TylerNowicki
Copy link
Contributor

  • We have encountered IR where it is necessary to transform a CallInst.
  • This requires a CallInst visitor. Although, a CallInst visitor can be
    added to the OpMap, it fails to find the CallInst visitor. Instead it
    gives up when it determines the Call is not an intrinsic or
    llvm-dialect op.
  • This change ensures we can find the CallInst visitor.

Copy link
Contributor

@tsymalla-AMD tsymalla-AMD left a comment

Choose a reason for hiding this comment

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

Thanks!
Can you please extend the IR-level tests as well, so we can see it handles IR with a call and a callbr properly, when there is neither a dialect op nor an intrinsic callback registered?

include/llvm-dialects/Dialect/OpMap.h Outdated Show resolved Hide resolved
test/unit/interface/OpMapIRTests.cpp Outdated Show resolved Hide resolved
@tsymalla-AMD tsymalla-AMD requested a review from Flakebi January 22, 2025 15:04
Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Looks good, some nitpicks inline

test/unit/interface/OpMapIRTests.cpp Outdated Show resolved Hide resolved
test/unit/interface/OpMapIRTests.cpp Outdated Show resolved Hide resolved
include/llvm-dialects/Dialect/OpMap.h Show resolved Hide resolved
* We have encountered IR where it is necessary to transform a CallInst.
* This requires a CallInst visitor. Although, a CallInst visitor can be
  added to the OpMap, it fails to find the CallInst visitor. Instead it
  gives up when it determines the Call is not an intrinsic or
  llvm-dialect op.
* This change ensures we can find the CallInst visitor.
@TylerNowicki TylerNowicki force-pushed the amd/dev/tnowicki/callinst.matching branch from fdd184a to 7865204 Compare January 22, 2025 16:39
@TylerNowicki
Copy link
Contributor Author

when there is neither a dialect op nor an intrinsic callback registered?

I don't understand this part of your request.

Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

If I understand Thomas correctly, he means using a visitor with a CallInst in the example here: https://github.com/GPUOpen-Drivers/llvm-dialects/blob/dev/example/ExampleMain.cpp
I think that makes sense

test/unit/interface/OpMapIRTests.cpp Outdated Show resolved Hide resolved
@TylerNowicki
Copy link
Contributor Author

TylerNowicki commented Jan 22, 2025

If I understand Thomas correctly, he means using a visitor with a CallInst in the example here: https://github.com/GPUOpen-Drivers/llvm-dialects/blob/dev/example/ExampleMain.cpp I think that makes sense

At this point, from what I can see, we don't have any Visitor tests... it would help if there was an existing I can work from, I guess I will have to create that...

Oh, do you mean, add the CallInst to the example?

@Flakebi
Copy link
Member

Flakebi commented Jan 22, 2025

Oh, do you mean, add the CallInst to the example?

Yeah, I meant that. And then print something to out and check it in the test like for the intrinsics.

@TylerNowicki
Copy link
Contributor Author

Oh, do you mean, add the CallInst to the example?

Yeah, I meant that. And then print something to out and check it in the test like for the intrinsics.

I guess its done... I don't really get it what this is doing.

@Flakebi
Copy link
Member

Flakebi commented Jan 22, 2025

I don't really get it what this is doing.

Ah, you need to add IR and checks here as well (and probably in the other test file): https://github.com/GPUOpen-Drivers/llvm-dialects/blob/dev/test/example/visitor-basic.ll

@tsymalla-AMD
Copy link
Contributor

Yeah. Sebastian understood correctly, please extend the visitor-basic.ll test + ExampleMain so that we can see the visitor visiting a generic CallInst.

@tsymalla-AMD
Copy link
Contributor

I guess its done... I don't really get it what this is doing.

The Example app is invocated during testing. It runs the visitor declared there against a given input LL file and then filechecks against the output there - so it checks if the callbacks are properly invoked.

@TylerNowicki
Copy link
Contributor Author

Done

@tsymalla
Copy link
Contributor

Looks good to me

Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Nice!

@Flakebi Flakebi merged commit 555a12c into GPUOpen-Drivers:dev Jan 23, 2025
8 checks passed
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.

4 participants