Skip to content

Fix top LLVM: renamed NVPTX barrier intrinsics. #8631

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mcourteaux
Copy link
Contributor

@mcourteaux mcourteaux commented May 22, 2025

llvm/llvm-project#140615 was temporarily reverted (but we haven't pulled yet) and will be relanded soon here llvm/llvm-project#141143.
This PR introduces a bit of logic that handles both intrinsic naming possibilities, which does not depend on LLVM version, but just tries both, to assure this doesn't break in any LLVM version. I tried locally on my non-updated LLVM that this still works. We can remove this change once we drop support for LLVM 19.

This patch is confirmed to work in the build bots here: #8629, but I'm opening a separate PR to separate the concerns.

Can be merged without testing I believe. @steven-johnson or @abadams, add the build bots for review is you want to.

@mcourteaux mcourteaux added the build Issues related to building Halide and with CI label May 22, 2025
@mcourteaux mcourteaux changed the title Fix top llvm renaming NVPTX barrier intrinsics. Fix top LLVM: renamed NVPTX barrier intrinsics. May 22, 2025
llvm::Function *barrier0 = module->getFunction("llvm.nvvm.barrier0");
internal_assert(barrier0) << "Could not find PTX barrier intrinsic (llvm.nvvm.barrier0)\n";
builder->CreateCall(barrier0);
llvm::Function *barrier = module->getFunction("llvm.nvvm.barrier0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: add a comment that specifically says "this can be removed once we no longer support LLVM19 or earlier"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that, and reordered the flow of the checks to favor the up-to-date check first.

@abadams
Copy link
Member

abadams commented May 23, 2025

I'm surprised this worked, because I thought module->getFunction will get declarations, regardless of whether or not the llvm backend will eventually provide definitions for them. Is it possible the new name has worked for a while?

@alexreinking
Copy link
Member

Shouldn't this be guarded by preprocessor macros, anyway?

@mcourteaux
Copy link
Contributor Author

I'm surprised this worked, because I thought module->getFunction will get declarations, regardless of whether or not the llvm backend will eventually provide definitions for them. Is it possible the new name has worked for a while?

Well, the original code on main fails with that function returning null. So it seems that this is not considering the declarations we have in ptx_dev.ll. But on the other hand... it still didn't return the new intrinsic when I didn't have it declared in ptx_dev.ll. Based on what I just experienced, it seemed like llvm::Module::getFunction() only returns a function if it both is declared and it has an implementation.

@mcourteaux
Copy link
Contributor Author

Under LLVM 19, the invalid PTX is indeed quite invalid...

	{ // callseq 2, 0
	.param .b32 param0;
	st.param.b32 	[param0], 0;
	call.uni 
	llvm.nvvm.barrier.cta.sync.aligned.all, 
	(
	param0
	);
	} // callseq 2

Seems like it does return the declaration...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to building Halide and with CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants