<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 20, 2012 at 9:51 AM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank" class="cremed">eli.friedman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Mon, Aug 20, 2012 at 3:37 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="cremed">chandlerc@gmail.com</a>> wrote:<br>

</div><div><div class="h5">> Hello all! I've been busy working on a somewhat larger patch than usual, and<br>
> Duncan indicated I should get a copy out to the list. This is essentially<br>
> the re-thinking of my approach after this discussion:<br>
> <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120716/146864.html" target="_blank" class="cremed">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120716/146864.html</a><br>
> I want to emphasize this is still a bit rough around the edges. =]<br>
><br>
><br>
> There are several very serious problems with SROA today:<br>
><br>
> - It is subject to the bane of my existence in optimizations: arbitrary<br>
> thresholds.<br>
> - It is overly conservative about what all constructs can be SROA-ed.<br>
> - The vector-value conversion is layered on top of the splitting logic,<br>
> missing many opportunities.<br>
> - It splits based the underlying type of the alloca, regardless of how the<br>
> alloca is accessed.<br>
> - When the splitting fails (often due to thresholds), the vector-value<br>
> conversion kicks in for preposterous cases to form "bit vectors" with i1024<br>
> and other large types.<br>
><br>
> Let's talk about the i1024 stuff first. This is actually what motivated me<br>
> to start looking at this space. There are a few PRs about how this causes<br>
> the backend to essentially give up and go home. In many other cases, such as<br>
> essentially every obvious implementation of sha1 or other cryptographic<br>
> hashing algorithm, instead the backend makes a heroic effort and recovers<br>
> from these i1024 values. The cost is in compile time. For a sha1<br>
> implementation recently posted as having compile times 2x that of GCC,<br>
> essentially all of the time is spent computing the demanded bits for the<br>
> i1024 values flying all over the code. No matter how heroic the backend<br>
> gets, analyzing i1024 is fundamentally expensive given the structure of<br>
> APInt and various analysis passes. We also can't just disable this aspect of<br>
> the SROA pass because *it's the only thing we have* for arrays of more than<br>
> 8 elements. This is particularly bad whenever the frontend lowers the alloca<br>
> as an 'i8' array, because that means that at 9 bytes, SROA shuts off except<br>
> for this conversion pass.<br>
><br>
> Next let's talk about splitting based on the underlying type. What happens<br>
> with C++ unions? Bad code. ;] In many cases, one side of the union ends up<br>
> getting nuked as dead code, and by only attending to the *use* of the<br>
> memory, we catch this by design. Another interesting aspect of designing the<br>
> analysis around the *use* of the memory is that it suddenly applies equally<br>
> well to malloc'ed memory.<br>
><br>
><br>
> Ok, so an overview of the design of the now pass.<br>
><br>
> First, credit to Duncan Sands. He and I were chatting on IRC, and I don't<br>
> remember who mentioned it first, but this design is the result of our<br>
> discussion.<br>
><br>
> The idea is to build a partitioning of the allocated memory by walking the<br>
> uses of that memory and observing the region of memory touched by the use.<br>
> We can then build mini-use-lists of the operations which actually refer to<br>
> each partition. From these we can rewrite each partition as its own alloca<br>
> and its users to refer directly to that alloca. The type of the alloca can<br>
> be computed by examining the users of the partition, etc. This is<br>
> complicated because some uses of memory can be *split*, while others cannot.<br>
> Take the canonical example: memcpy. We can split a memcpy into two or three<br>
> smaller memcpys at will. However, we cannot (in the general case) so easily<br>
> split a load or a store. More importantly, we likely would not wish to split<br>
> a load or a store as the point of SROA is to make SSA-able values. Thus what<br>
> we really want to do is build a partitioning that attempts to assign a<br>
> disjoint partition to each collection of overlapping unsplittable uses. The<br>
> splittable uses will then be split as necessary to satisfy this<br>
> partitioning.<br>
><br>
> The result is a pass which can handle arbitrarily large arrays and<br>
> structures and split single value loads and stores out of these large<br>
> allocas by splitting up any large memcpy or memsets. The code growth is<br>
> generally limited by the number of these partitions formed, not by the size<br>
> of the original buffer. The number of partitions is a function of the number<br>
> of input instructions. This prevents the scaling problems that initially<br>
> caused the thresholds in the current pass. However, building these<br>
> partitions is not easy. The new pass is significantly larger and has some<br>
> rather complex parts. On the whole, I suspect it is worth it to get the<br>
> increased power and flexibility.<br>
><br>
> The other major change is that I have tried to very heavily use InstVisitor<br>
> to drive the pass rather than writing out large loops with nested dyn-cast<br>
> chains. While the InstVisitor often ends up being significantly more lines<br>
> of code, I think the code is much better factored as a consequence.<br>
><br>
><br>
> I've attached a patch file, but really there isn't much of a patch. It's<br>
> brand new code. Probably the best way to read it is over on github:<br>
> <a href="https://github.com/chandlerc/llvm-hacking/blob/sroa-rewrite/lib/Transforms/Scalar/SROA.cpp" target="_blank" class="cremed">https://github.com/chandlerc/llvm-hacking/blob/sroa-rewrite/lib/Transforms/Scalar/SROA.cpp</a><br>

><br>
> There are still two major missing features:<br>
> 1) rewriting overlapping accesses which look like element accesses on a<br>
> vector to actually be element accesses on a vector<br>
> 2) adding an SSAUpdater mode for promotion<br>
><br>
> #1 I'm currently working on.<br>
> #2 I would prefer to do in-tree as I would rather have it be obvious as the<br>
> code moves from one pass to the other. I don't expect any really significant<br>
> changes to this logic.<br>
><br>
> I know that Ben has already read quite a bit of the implementation code, so<br>
> CC-ing him. Any comments or early review much appreciated.<br>
<br>
</div></div>It looks like you're missing handling for debug intrinsics.<br></blockquote><div><br></div><div>And lifetime intrinsics. Like I said, it's not complete. The big question is when it is complete enough to go into the tree under a flag, vs. on by default, etc.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I don't think your usage of inbounds for computing properties of GEPs is safe.<br></blockquote><div>
<br></div><div>Nope. It's horribly broken. Thanks for pushing me on this issue, I had a misunderstanding of how GEPs worked, because I assumed rather than reading the spec carefully.</div><div><br></div><div>Funny thing? My assumption was based on the existing SROA pass which also gets this wrong:</div>
<div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">#include <stdio.h><br>struct S {<br>
  int x;<br>  int __attribute__((vector_size(16))) v;<br>  int y;<br>};<br>struct S f(int a, int b, int c, int i) __attribute__((noinline)) {<br>  struct S s;<br>  volatile int q = c;<br>  s.x = a;<br>  ((int*)&s.v)[i] = b;<br>
  s.y = q;<br>  return s;<br>}<br>int main() {<br>  struct S s = f(1, -1, 3, -1);<br>  printf("%d %d\n", s.x, s.y);<br>}</blockquote><div><br></div><div>Clang miscompiles this program at -O2 because of SROA I think... At least, the LLVM IR produced seems valid according to the spec of the IR, and the transform applied by SROA seems invalid -- the index of -1 doesn't overflow anything, it just walks past the end of a sequential type, which everything indicates is allowed.</div>
<div><br></div><div>We happen to be getting away with this because this transform in the existing SROA is only ever applied to vectors, never to arrays or pointers, or anything that happens to be lowered as an array or pointer.</div>
<div><br></div><div>I'm inclined to completely remove this transform. It seems fundamentally unsafe given the current spec of GEPs, and provides fairly limited benefit if the frontend consistently lowers to insertelement and extractelement (which it did in most of my experiments). If we want to support this transform, I think we need to either extend GEP to have an optional constraint on inbounds indexing within array and/or vector aggregates, or we need to change the semantics of the existing 'inbounds' keyword to mean that.</div>
</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">How does your code handle accesses to following union?<br>
<br>
union DoubleBits {<br>
  double x;<br>
  int y[2];<br>
};<br></blockquote><div><br></div><div>Hmm, interesting test case. It introduces a bitcast that disrupts everything. Let me look at this and related stuff and try to handle it reasonably.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
I'm sort of worried about allowing SROA so much freedom in terms of<br>
splitting memset/memcpy; your algorithm can turn a single memcpy into<br>
an arbitrary number of memcpys (which is a quadratic explosion in<br>
codesize in the worst case).  Not sure if it's an issue in practice.<br></blockquote><div><br></div><div>Yes, this is the primary concern with the new algorithm. That said, the N*M which explodes requires N overlapping splittable operations which overlap with M disjoint un-splittable operations. My hope is that this latice structure is very rare. Even better, whenever we start heavily splitting, we're getting something out of it -- we're successfully isolating un-splittable partitions which should form candidates for promotion.</div>
<div><br></div><div>Do you have any ideas for mitigating or bounding the growth that don't devolve to heuristics?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
Minor nit:<br>
<br>
    if (SI.getOperand(0) == U.Ptr)<br>
      return false;<br>
<br>
This is overly tricky; SI.getOperand(1) != U.Ptr, please.<br></blockquote><div><br></div><div>Done.</div></div></div>