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

[CIR][CUDA] Register __global__ functions #1441

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

Conversation

AdUhTkJm
Copy link
Contributor

@AdUhTkJm AdUhTkJm commented Mar 5, 2025

This is part 2 of CUDA lowering. Still more to come!

This PR generates __cuda_register_globals for functions only, without touching variables.

It also fixes two discrepancies mentioned in Part 1, namely:

  • Now CIR will not generate registration code if there's nothing to register;
  • __cuda_fatbin_wrapper now becomes a constant.

voidPtrTy = PointerType::get(voidTy);
voidPtrPtrTy = PointerType::get(voidPtrTy);
intTy = typeSizeInfo.getIntType(&getContext());
charTy = typeSizeInfo.getCharType(&getContext());
Copy link
Member

Choose a reason for hiding this comment

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

Suggestions:

  • Make CIRDataLayout have a typeSizeInfo data member caching it.
  • Remove all the variables and (a) just instantiate the types whenever these variables are being used (there aren't many of them) or (b) ask typeSizeInfo from CIRDataLayout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some lowering test cases doesn't have a TypeSizeInfoAttr in it, so I added a if-statement to guard against this situation.

Copy link
Member

Choose a reason for hiding this comment

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

Were those testcases directly written in CIR? If so you should just add the attribute to them

Copy link
Contributor Author

@AdUhTkJm AdUhTkJm Mar 7, 2025

Choose a reason for hiding this comment

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

There are 42 of them, so it seems not quite feasible to me.

Shall we leave it as it is for now, and tidy these test cases up later?

Copy link
Member

Choose a reason for hiding this comment

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

All of them fail? You need to update only the ones that fail. Unfortunately we don't want to build up even more technical debt!

Copy link
Contributor Author

@AdUhTkJm AdUhTkJm Mar 8, 2025

Choose a reason for hiding this comment

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

Yeah, all 42 ones fail, as they just say module { without any attributes. I'll add them up then.

Edit: Now it's done and 69 test files has been changed to add the attribute.

@AdUhTkJm AdUhTkJm force-pushed the ctordtor branch 3 times, most recently from a82236a to 056f658 Compare March 7, 2025 15:51
@AdUhTkJm AdUhTkJm force-pushed the ctordtor branch 2 times, most recently from 734e244 to de26454 Compare March 8, 2025 17:39
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