Added optimization pass to convert heap-based allocs to stack-based allocas.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Transforms/BufferOptimizations.cpp | ||
---|---|---|
92 | Could this just be a bunch of static helper functions? What is the benefit to put this into a class? | |
95 | Avoid auto if it does not safe much typing. | |
118 | This should be a pass option. | |
127 | This is sufficient if the allocation scope is the top-level scope. Otherwise, the use could also be in some further up parent of the scope. You need to query the ancestor relationship here. | |
289 | It is safer to query the result type for whether it is static. One could have an alloc operation that returns dynamic memrefs even without operands. | |
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
610 | I would not want this transformation to happen, as might quickly exhaust the stack space. Using alloca inside of a loop is dangerous. Rather, could the optimization be limited to cases where the allocation is not inside a loop? In essence, if the alloc cannot be moved to the level of the allocation scope (conceptually), it should not be transformed into an alloca. This is just conceptually, as alloca should not be moved out of conditionals either. |
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
---|---|---|
610 | In the transformation process, the parent allocation scopes are checked. During this traversal, the visited operations are checked to be a known control flow interface. If this is the case, we skip the transformation for the given alloc value. |
mlir/lib/Transforms/BufferOptimizations.cpp | ||
---|---|---|
45 | Thinking about this more, I believe the idea here is that if the value escapes, then there must be at least one alias at the level of the allocation scope, so this will return true. Even if it ignores aliases that are already outside of the allocation scope. Is that correct? Maybe add a comment? | |
53 | Please update the comment to reflect that it only returns an allocation scope if a suitable one can be found, i.e., one that is not outside potentially looping control flow. | |
62 | This is not sufficient. Rather, if we do not know the ops control flow (does not implement RegionBranchOpInterface) or if it can have loop-like behavior (the RegionBranchOpInterface has a cycle in the region dependencies), we cannot do this. If it has non-looping behavior (like scf.if) it is ok to transform into an alloca but the alloca should not be hoisted out of the conditional. | |
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
552 | Please also fix the explanatory comment. |
mlir/lib/Transforms/BufferOptimizations.cpp | ||
---|---|---|
65 | This was an oversight in the previous review. I think you generally want a mightBeLoop that also returns true of the operation does _not implement RegionBranchOpInterface_. We have to be conservative when doing these optimizations (this one and the other two hoisting optimizations, as well). Adding a test for this would be great. | |
mlir/test/Transforms/promote-buffers-to-stack.mlir | ||
13 | Nit: please update comment. | |
115 | Nit: here, too. And also for the changes below. |
Thinking about this more, I believe the idea here is that if the value escapes, then there must be at least one alias at the level of the allocation scope, so this will return true. Even if it ignores aliases that are already outside of the allocation scope.
Is that correct? Maybe add a comment?