[PATCH] D137707: Move "auto-init" instructions to the dominator of their users

serge via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 2 08:23:13 PST 2023


serge-sans-paille added a comment.

In D137707#4164689 <https://reviews.llvm.org/D137707#4164689>, @nikic wrote:

> High level notes:
>
> - I'm not sure I fully understand the motivation for using MemorySSA here. Unless I'm missing something, It seems like when it comes to optimizing allocas, it would be sufficient to look at non-transparent users of the alloca (by which I mean, in first approximation, look through bitcast, gep, count everything else). The review mentions fences, but wouldn't these be covered as long as you consider pointer captures as "uses"?

I could cowardly let @efriedma answer there, because he motivated this change. You can refer to https://reviews.llvm.org/D137707#3945415 for his initial thoughts.

> - This is missing some tests where there are other memory instructions beyond the auto-init stores. You should find that, as implemented, these are going to block the transform even if they are "obviously" unrelated. If they are MemoryUses, you can recover this by requesting optimized use form. However, MemoryDefs always depend on the preceding MemoryDef, even if they are non-clobbering. This means that your optimization will always stop at the next MemoryDef, even if it is to a different alloca. This is a big limitation. You could do alias checks to skip those, but that's also the point where this becomes an expensive transform.

Yeah, test coverage is currently not sufficient, I'll add some and get a better understanding of that particular part.

> - There is a pending patch to add sinking support to DSE (https://reviews.llvm.org/D136218). I haven't really looked at it yet, but I expect it does the MSSA-based transform in the right way (i.e. skipping non-clobbering instructions). This brings me back to the first point...

Thanks for sharing!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137707/new/

https://reviews.llvm.org/D137707



More information about the llvm-commits mailing list