This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenMP] Insert alloca for kernel args at function entry block instead of the launch point.
ClosedPublic

Authored by dhruvachak on Mar 10 2023, 11:44 AM.

Details

Summary

If an inlined kernel is called in a loop, the launch point alloca would
lead to increasing stack usage every time the kernel is invoked. This
could make the application run out of stack space and crash. This problem
is fixed by using the alloca insertion point while creating the alloca instruction.

Fixes https://github.com/llvm/llvm-project/issues/60602

Diff Detail

Event Timeline

dhruvachak created this revision.Mar 10 2023, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 11:44 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dhruvachak requested review of this revision.Mar 10 2023, 11:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

TODOs: Update existing LIT tests, add a new one.

No tests updated?

No tests updated?

That's a TODO, coming soon.

dhruvachak retitled this revision from Insert alloca for kernel args at function entry block instead of the launch point. to [Clang][OpenMP] Insert alloca for kernel args at function entry block instead of the launch point..Mar 10 2023, 11:54 AM

Check the other functions around, they take an AllocaIP, which is on the user side not necessarily the entry block. We need to follow that scheme here too.

Addressed comment. Using the alloca insert point for kernel args alloca.

dhruvachak edited the summary of this revision. (Show Details)Mar 13 2023, 12:04 AM

Fixed LIT test failure, added new clang test OpenMP/bug60602.cpp.

jdoerfert accepted this revision.Mar 14 2023, 2:21 PM

LG, make sure all tests pass, this one seems to fail according to build kite: OpenMP/target_map_codegen_hold.cpp

clang/test/OpenMP/target_map_codegen_hold.cpp
211

The hash is hardcoded. I recommend removing this part and putting the old check lines in again.

This revision is now accepted and ready to land.Mar 14 2023, 2:21 PM

Fixed clang test OpenMP/target_map_codegen_hold.cpp.

dhruvachak marked an inline comment as done.Mar 16 2023, 11:22 PM
dhruvachak added inline comments.
clang/test/OpenMP/target_map_codegen_hold.cpp
211

@jdoerfert Thanks for the suggestion. It was passing for me locally, I assume the locally built compiler was generating the same hash. I removed the hash lines like it was earlier, it should pass now.

dhruvachak updated this revision to Diff 506107.EditedMar 17 2023, 9:20 AM
dhruvachak marked an inline comment as done.

Removed attributes (as original) from clang test OpenMP/target_map_codegen_hold.cpp.

Rebased.