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][CIRGen] Add dynamic builtin alloca intrinsics support #547

Merged

Conversation

orbiri
Copy link
Collaborator

@orbiri orbiri commented Apr 16, 2024

This patch adds the CIRGen for the following builtin functions:

  • alloca;
  • _alloca;
  • __builtin_alloca;
  • __builtin_alloca_uninitialized.

Missing support to add in the future:

  • Non-default auto initialization setting. The default is to not initialize the allocated buffer, which is simpler to implement. This commit is leaving the skeleton to implement this feature following clang's codegen pattern.
  • It may be possible that the frontend has set non-default address space for the alloca's return value. This is the case for OpenCL or AMDGPU codes for example. This is handled in clang codegen via address space cast, and is left for future implementation. This commit introduces a guard-rail around this behaviour.

P.S.:
I'm aware this is rather an "esoteric" feature to be supported and I'm sure there are more valuable features to implement before that! Implementing this feature was my method of learning more about the development flow for CIR - working along side clang's codegen, testing and debugging flow, etc.

@orbiri orbiri force-pushed the orbiri/dynamic-builtin-alloca-support branch 2 times, most recently from 44b4d75 to 0203a7c Compare April 16, 2024 20:02
@orbiri
Copy link
Collaborator Author

orbiri commented Apr 16, 2024

Embarrassing formatting and lit-tests errors fixed and verified locally 🤦

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for your first PR, looking forward to see more contributions :)

I'm aware this is rather an "esoteric" feature to be supported and I'm sure there are more valuable features to implement before that! Implementing this feature was my method of learning more about the development flow for CIR - working along side clang's codegen, testing and debugging flow, etc.

No sweat, feel free to work on whatever makes you excited. Btw, these are actually important, since they show up in base system libraries, which are crucial to get CIR support bottom up in common software stacks.

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/builtin-ms-alloca.c Show resolved Hide resolved
@orbiri
Copy link
Collaborator Author

orbiri commented Apr 17, 2024

Thanks for the review!! Will address the comments in the coming days.
New push is just rebasing on top of Nathan's rebase, didn't address any comment yet! :)

@orbiri orbiri force-pushed the orbiri/dynamic-builtin-alloca-support branch 2 times, most recently from 8ef26e6 to c5d3451 Compare April 20, 2024 14:19
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not initialize the
  allocated buffer, which is simpler to implement. This commit is leaving the
  skeleton to implement this feature following clang's codegen pattern.
- It may be possible that the frontend has set non-default address space for
  the alloca's return value. This is the case for OpenCL or AMDGPU codes for
  example. This is handled in clang codegen via address space cast, and is left
  for future implementation. This commit introduces a guard-rail around this
  behaviour.
@orbiri
Copy link
Collaborator Author

orbiri commented Apr 20, 2024

Addressed all comments, main changes:

@orbiri
Copy link
Collaborator Author

orbiri commented Apr 25, 2024

@bcardosolopes do you think we can land this? :)

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

LGTM

@bcardosolopes bcardosolopes merged commit 7b990b9 into llvm:main Apr 25, 2024
6 checks passed
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
lanza pushed a commit that referenced this pull request Nov 5, 2024
This patch adds the CIRGen for the following builtin functions:

- `alloca`;
- `_alloca`;
- `__builtin_alloca`;
- `__builtin_alloca_uninitialized`.

Missing support to add in the future:
- Non-default auto initialization setting. The default is to not
initialize the allocated buffer, which is simpler to implement. This
commit is leaving the skeleton to implement this feature following
clang's codegen pattern.
- It may be possible that the frontend has set non-default address space
for the alloca's return value. This is the case for OpenCL or AMDGPU
codes for example. This is handled in clang codegen via address space
cast, and is left for future implementation. This commit introduces a
guard-rail around this behaviour.
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