-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[LVA] Support begin_access in projections. #31132
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
Conversation
Adds support for projecting through begin_access instructions.
@gottesmm Can you please review this? |
@zoecarver I am pretty sure we don't want to have access markers be projections. A projections are meant to be like a gep semantically. begin_access semantically isn't that. @atrick your thoughts? |
It's not semantically ideal. You could think about it as a GEP 0 (get the pointer). There are a lot of places throughout the SILOptimizer that use projections. They would all (or most) need to be updated to essentially have two projections and do extra analysis about the access scopes. In this case, I think the performance, QoI, and lack of complexity outweigh the unfortunate semantics. |
Mem2reg, for example, would be one of the easier cases. All we'd need to do is update all the places we project things to also support access instructions. DSE and RLE, on the other hand, would need to compensate for the different behavior in alias analysis and create two projections, one before the begin access and one after. This would be a huge headache. |
@zoecarver I also think that no matter what we do, we can not just enable this everywhere. I would need to think about this a lot more and we would need to be very, very careful with this change. This could lead to potentially significant changes in codegen since a lot of code goes through here. |
An access marker is not a projection, mainly because it has side effects of its own. It's also a scoped instruction, which requires substantially different handling. It's also critical for a pass to be explicit about how it identifies memory locations. We have had, and continue to have lots of terrible bugs in AliasAnalysis because this has never been cleanly specified. Part of moving to OSSA should entail taking advantage of access markers. We should not delete access markers until after eliminating OSSA. This means passes should explicitly identifying memory by its formal access. We have basically usable APIs for this now: stripAccessMarkers, isAccessProjection, getAccessedAddress, AccessedStorage. Part of the difficulty here is that, for now, we want most passes to run in both OSSA and non-OSSA. So a good next step might be to gradually move the access marker elimination pass later in the pipeline. Missed optimizations will need to be fixed as that is done . But then a pass can be modified to assume access markers, and still run in both OSSA/non-OSSA. For the cases in which we don't want to use access markers to identify memory, I have a very simple AddressPath abstraction sitting on a branch. I can check it in literally as soon as I'm done with urgent bugs. RLE/DSE should use that abstraction instead. I objected to the use of ProjectionPath when those passes were designed, but the author promised to change the design as soon as the prototype was checked in (Hah!). Note that the Projection abstraction is perfectly fine, and AddressPath is just a simple wrapper for it. But one thing that AddressPath does for you is "see through" access markers and borrows, which is all you need. |
Fantastic. @atrick, that's perfect. I'll close this in favor of that change. |
There are some other passes that might benefit from using |
Adds support for projecting through begin_access instructions.