LLVM: add AggregateAllocaSplitting transformation#1649
Conversation
phate
changed the title
phate
force-pushed
the
RvsdgTreePrinter-SplitableAllocas
branch
3 times, most recently
from
e43c5b2 to
072fc75
Compare
phate
force-pushed
the
RvsdgTreePrinter-SplitableAllocas
branch
2 times, most recently
from
5a6940f to
9187240
Compare
phate
force-pushed
the
RvsdgTreePrinter-SplitableAllocas
branch
from
9870b70 to
e049378
Compare
|
@haved This one is now ready for review. |
haved
left a comment
haved
left a comment
There was a problem hiding this comment.
Looks good. I have some comments you can look at it you'd like. The code for finding all users of the alloca and all users of the GEP is very similar, so it makes me wish for a "reverse tracer" in the future
| namespace jlm::llvm | ||
| { | ||
|
|
||
| class AggregateAllocaSplitting final : public rvsdg::Transformation |
There was a problem hiding this comment.
A comment explaining what the class does would be nice
| : allocaNode(&allocaNode) | ||
| {} | ||
|
|
||
| rvsdg::SimpleNode * allocaNode = nullptr; |
There was a problem hiding this comment.
Since this value is assigned in the constructor I don't think it should have a default value. It should never actually be null?
| AllocaTraceInfo allocaTraceInfo(allocaNode); | ||
|
|
||
| util::HashSet<rvsdg::Output *> visited; | ||
| util::HashSet<rvsdg::Output *> toVisit{ &address }; |
There was a problem hiding this comment.
The tovisit HashSet could instead be a simple queue, with the HashSet instead storing "visited or queued". I often call that hash set "seen", but it is not a great name.
| "Unhandled owner region node type: ", | ||
| userRegion->node()->DebugString())); | ||
| // Silence compiler | ||
| return false; |
There was a problem hiding this comment.
Do we really need to return even after throwing? How would control flow continue after the throw? If it complains then by all means, I just don't understand why the compiler would give such a false positive
| bool hasOnlyLoadsAndStores = true; | ||
|
|
||
| util::HashSet<rvsdg::Output *> visited; | ||
| util::HashSet<rvsdg::Output *> toVisit{ &address }; |
There was a problem hiding this comment.
Same here, one hash set and one queue
| JLM_ASSERT(index0 == 0); | ||
|
|
||
| auto elementAlloca = elementAllocaNodes[index1]; | ||
| auto & routedAddress = rvsdg::RouteToRegion( |
There was a problem hiding this comment.
We do not memorize routing to regions here, so in the future it might be nice to do that to avoid giving extra work for CNE. Not a big issue now
2 participants
No description provided.