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

[SYCL] Move SYCLLowerWGLocalMemoryPass to OptimizerEarlyEPCallback #16347

Closed
wants to merge 4 commits into from

Conversation

jsji
Copy link
Contributor

@jsji jsji commented Dec 12, 2024

The pass transforms __sycl_allocateLocalMemory call to access of global
variable @WGLocalMem .

Move the transform to beginning of the optimizer since the access
could enable more optimization than the function call.

The pass transforms __sycl_allocateLocalMemory call to access of global
variable @WGLocalMem .

Move the transform to beginning of the optimizer since the access
could enable more optimization than the function call.

In addition, intel gpu compiler has a pass to transform global variable
in addrspace(3) to alloca, we must run SYCLLowerWGLocalMemoryPass before
it.
@jsji jsji self-assigned this Dec 12, 2024
@jsji jsji requested a review from wenju-he December 12, 2024 16:23
@jsji
Copy link
Contributor Author

jsji commented Dec 12, 2024

Similar to #16183 but tweak the position of the pass to satisfy the requirements.

@jsji jsji requested a review from bader December 12, 2024 16:25
@bader
Copy link
Contributor

bader commented Dec 12, 2024

@jsji, is this PR ready for review?

Thanks for putting the motivation for the change to the description. It sounds reasonable except this part:

In addition, intel gpu compiler has a pass to transform global variable
in addrspace(3) to alloca, we must run SYCLLowerWGLocalMemoryPass before
it.

DPC++ runs all passes before the translation to SPIR-V and IGC runs its passes after the translation to SPIR-V, so existing location satisfies the requirement already.

@jsji jsji marked this pull request as ready for review December 12, 2024 18:18
@jsji jsji requested review from a team as code owners December 12, 2024 18:18
Update comments.

Co-authored-by: Victor Lomuller <[email protected]>
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

FE change looks ok to me but I do not know the functionality well enough to approve the new pass location. I defer to @bader for approval

@@ -1043,7 +1043,14 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
/*ExcludeAspects=*/{"fp64"}));
MPM.addPass(SYCLPropagateJointMatrixUsagePass());
});
else if (LangOpts.SYCLIsHost && !LangOpts.SYCLESIMDBuildHostCode)
PB.registerOptimizerEarlyEPCallback([](ModulePassManager &MPM,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to use registerOptimizerEarlyEPCallback instead of registerPipelineStartEPCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in https://github.com/intel/llvm/pull/16183/files/43526397fa2b4c559666fdfd6f302bfba7409255#r1857546652, we need to make sure this pass run after AlwaysIniner, moving to PipelineStart is what was tried in #16183, it is before alwaysinliner and too early.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsji, could you please extend the comment at lines 1049-1050 with the requirement to run AlwaysInliner pass before SYCLLowerWGLocalMemoryPass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure.

@wenju-he
Copy link
Contributor

wenju-he commented Dec 13, 2024

If we are sure that the pass needs to be moved to an earlier stage, I had an idea that we can remove the pass's implicit dependency on alwaysinliner pass and move the pass to pipeline start: wenju-he@e6ce584
Please review if it makes sence.

@jsji
Copy link
Contributor Author

jsji commented Dec 13, 2024

If we are sure that the pass needs to be moved to an earlier stage, I had an idea that we can remove the pass's implicit dependency on alwaysinliner pass and move the pass to pipeline start: wenju-he@e6ce584 Please review if it makes sence.

Thanks @wenju-he ! Yes, looks good idea to handle the inlining of these two functions inside the pass itself. Please post your PR for review instead.

@wenju-he
Copy link
Contributor

Please post your PR for review instead.

#16356

@jsji
Copy link
Contributor Author

jsji commented Dec 19, 2024

Close, use #16356 instead.

@jsji jsji closed this Dec 19, 2024
@jsji jsji deleted the wglocalearly branch December 19, 2024 02:58
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.

5 participants