-
Notifications
You must be signed in to change notification settings - Fork 99
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 const attribute to alloca operations #892
Conversation
0b45aea
to
e58e514
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this patch deal with constant reference and constexpr?
// - If there is a load operation that properly dominates it, replace the | ||
// load with that dominator load. This process is "recursive": if load A | ||
// dominates load B and load B dominates load C, we should eventually | ||
// replace load C with load A. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we records A dominate C directly?
Constant reference is not yet taken care of in this patch, I'll add it later!
Well since |
In Decl, we have |
Sounds good to me, I'll add a test along with the update for references. |
e58e514
to
ad9f9ec
Compare
Two updates:
|
ad9f9ec
to
a5f0feb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on cool ideas @Lancern!
I think we can split this patch into two: (1) introduce the const + CIRGen + tests and (2) optimization on top of new alloca attribute.
For (2), I wonder if you tried to explore the path of teaching more traditional optimizations about these new introduced properties (const
in the example here). For example, a combo of:
- -mem2reg
- -sccp
- -remove-dead-values
- canonicalize...
I believe that in principle we should implement these hooks and try to get these optimizations for free from MLIR before we try to develop custom CIR ones. I wonder if you found any limitations while exploring those.
Not quite, I have only tried the combination with mem2reg. The optimization in this PR is quite orthogonal to mem2reg although they both optimize some simple cases like: int produce_int();
int test() {
const int x = produce_int();
int a = x;
int b = x;
return a + b;
} In this simple case, since int produce_int();
int test() {
const int x = produce_int();
return x + x;
} However, once the address of int produce_int();
void consume(const int &);
int test() {
const int x = produce_int();
int a = x;
consume(x);
int b = x;
return a + b;
} Since the allocation for
OK I'll split it later. I may draft a more detailed RFC along with PR (2) so we could all get a feel about the range and impact of this cool optimization. |
It's orthogonal but my point is that compiler optimizations usually work with a combination of multiple passes and not adhoc passes that do all work and analysis at once.
Are you saying mem2reg can generate transformations that allows this to be optimized without any of the changes from this PR?
I understand where you are coming from and what you want to achieve, but I'm a bit worried about making assumptions about memory in adhoc fashion, without for example, the help of a proper alias analysis to feed in this information. In general, what I'm trying to convey is that we should first start implementing the hooks for the existing passes MLIR provides and slowly enable them in our pipeline. Putting to the context of this PR, I'd like to see how |
For the simple case I shown in the previous comment, yes. But for more complex examples, we have to come up a way to teach mem2reg (or any other existing optimizations) about the constness added in this PR.
I get your idea. You're conveying that after landing the constness attribute, a more practical way to make it useful is to first try teach existing MLIR optimizations about the constness and see what they could already do. Do I understand it correctly? |
Updated, removed the transformation pass from this PR. |
Neat, might be worth adding that testcase to current mem2reg tests.
This would be one interesting path to go along, yes. There are many possible paths though:
One concern I have with the existing PR approach is that dominance checks can get expensive, you might need more caching or more conservative assumptions, maybe looking into how LLVM eliminate redudant loads can provide you with a few more insights on how these opts usually operate to be efficient. Another caveat here is that ClangIR is currently WIP building bigger codebases / benchmarks, it's probably gonna get easier to get measurements / evaluate optimizations once we have a baseline for correctness and compile time. |
This patch adds a new attribute `const` to the alloca operation to indicate that the corresponding local variable declaration is `const`-qualified. Future optimizations may find this new attribute useful.
Rebased onto the latest |
This PR tries to give a simple initial implementation for eliminating redundant loads of constant objects, an idea originally posted by OfekShilon. Specifically, this PR adds a new unit attribute `const` to the `cir.alloca` operation. Presence of this attribute indicates that the alloca-ed object is declared `const` in the input source program. CIRGen is updated accordingly to start emitting this new attribute.
This PR tries to give a simple initial implementation for eliminating redundant loads of constant objects, an idea originally posted by OfekShilon.
Specifically, this PR adds a new unit attribute
const
to thecir.alloca
operation. Presence of this attribute indicates that the alloca-ed object is declaredconst
in the input source program. CIRGen is updated accordingly to start emitting this new attribute.