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] Make use of !invariant.group metadata for const allocas #1159

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

Conversation

Lancern
Copy link
Member

@Lancern Lancern commented Nov 24, 2024

This PR updates the LLVM lowering part of load and stores to const allocas and makes use of the !invariant.group metadata in the result LLVM IR.

The HoistAlloca pass is also updated. The const flag on a hoisted alloca is removed for now since their uses are not always invariants. Will update in later PRs to teach their invariants.

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.

Overall looks good to me, thanks for the incremental approach! One remaining question: should this be enabled only -O1+ or do we want this for -O0 as well?

@Lancern
Copy link
Member Author

Lancern commented Nov 26, 2024

One remaining question: should this be enabled only -O1+ or do we want this for -O0 as well?

I think we should emit the metadata under -O1+ to prevent LLVM from any possible optimization under -O0.

And I realized that LowerToLLVM code does not have access to the code generation option which keeps the optimization flags. Sent #1171 for this and I'm marking this PR as draft until that PR lands.

@Lancern
Copy link
Member Author

Lancern commented Dec 3, 2024

Rebased onto the latest main.

@Lancern Lancern marked this pull request as ready for review December 3, 2024 14:09
This patch updates the LLVM lowering part of load and stores to const allocas
and makes use of the !invariant.group metadata in the result LLVM IR.

The HoistAlloca pass is also updated. The const flag on a hoisted alloca is
removed for now since their uses are not always invariants. Will update in later
patches to teach their invariants.
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 pending some comment update

alloca->moveBefore(insertPoint);
if (alloca.getConstant()) {
// For now, we remove the const flag on nested allocas for constant
// variables when hoisting them.
Copy link
Member

Choose a reason for hiding this comment

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

In theory the alloca is still const, this is a workaround for the invariant side of things that are going to be generated later, in theory we could be propagating another attribute that has the specific semantic and delete it instead. I'm not sure it's worth adding that right now though. Can you explain that in the comment instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

in theory we could be propagating another attribute that has the specific semantic and delete it instead

I'm wondering recently if we could leverage the still-missing CIR lifetime markers as pointed out in #1129 . The rough idea is the constness property would only hold in the program points between the lifetime start and end markers, and then we don't have to remove the const flag here. What do you think?

Anyway, this would not block this PR. Will update the comment here later.

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