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

Add dynamic shared memory allocation #144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

michael-kenzel
Copy link
Contributor

@michael-kenzel michael-kenzel commented Aug 4, 2023

This adds a parameter to allocate a given amount of dynamic shared memory upon kernel launch.

corresponding runtime changes: AnyDSL/runtime#41

@Hugobros3
Copy link
Contributor

Hugobros3 commented Aug 5, 2023

This PR looks fine but keep in mind my remarks concerning computing that shared memory pool size statically using PE instead of a bespoke pass.

Also why merge this into master considering the plugin system isn't merged there currently and probably won't be for a while ?

@michael-kenzel
Copy link
Contributor Author

michael-kenzel commented Aug 5, 2023

Sorry, I should have mentioned that up there, this is independent of the plugin and programming model stuff. It just adds a parameter to allocate a given amount of dynamic shared memory upon launch. There's a corresponding PR that updates the runtime accordingly. We will need this regardless of how we end up computing the size, and this is also useful as just a standalone thing, so we thought it would be best to just get this into main right away to minimize divergence.

@michael-kenzel michael-kenzel marked this pull request as draft August 9, 2023 12:27
@michael-kenzel michael-kenzel force-pushed the mikey/smem branch 3 times, most recently from 0509bd9 to 00bb7bd Compare August 25, 2023 01:09
@michael-kenzel michael-kenzel force-pushed the mikey/smem branch 2 times, most recently from c02028e to bb992d8 Compare August 1, 2024 04:56
@michael-kenzel michael-kenzel marked this pull request as ready for review August 1, 2024 18:00
@@ -76,4 +76,8 @@ llvm::Value* AMDGPUCodeGen::emit_reserve(llvm::IRBuilder<>& irbuilder, const Con
return emit_reserve_shared(irbuilder, continuation, true);
}

llvm::Value* AMDGPUCodeGen::emit_local_memory(llvm::IRBuilder<>& irbuilder, const Continuation* continuation) {
return emit_local_memory_base_ptr(irbuilder, continuation);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this identical function copy/pasted here and in nvvm ? since there are no alternative implementations there's no reason to do this, just have the default impl in llvm.cpp be this one

Copy link
Contributor Author

@michael-kenzel michael-kenzel Aug 5, 2024

Choose a reason for hiding this comment

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

I just did it the same way it was done for reserve_shared(). It's not valid to use this function unless one of the GPU backends is being used. So the base implementation in llvm.cpp emits an error, and the GPU backends override that with a call to the actual implementation.

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.

2 participants