<div dir="ltr">ping ?!??<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-26 21:32 GMT-07:00 deadal nix <span dir="ltr"><<a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a>></span>:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div>Hi,<br><br></div>So I updated the patch to include more sanity checks and tests for load past the end of the store.<br>

<br>CoerceAvailableValueToLoadType will return null if it doesn't know how to do a bitcast between the values provided, or if the load is larger than the store.<br>
<br></div>My understanding is that<br><br>@@ -1930,6 +1947,11 @@ bool GVN::processLoad(LoadInst *L) {<div class=""><br>       if (DL) {<br>         StoredVal = CoerceAvailableValueToLoadType(StoredVal, L->getType(),<br>

                                                    L, *DL);<br>
<br></div></div>This code is not calling GetStoreValueForLoad because it knows it won't need the bit manipulations of the more general case. This assertion is true when you deal with basic types, but do not hold anymore for struct/array. It still seems worthwhile to try the fast path as it should be the taken most of the time.<br>


</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-26 4:24 GMT-07:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<div><div class="h5">

<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>----- Original Message -----<br>
> From: "deadal nix" <<a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a>><br>
</div><div>> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>, "Owen Anderson" <<a href="mailto:owen@apple.com" target="_blank">owen@apple.com</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>><br>



> Sent: Wednesday, June 25, 2014 2:05:41 PM<br>
> Subject: Re: Optimize load from aggregate stores<br>
><br>
><br>
><br>
</div><div><div>> Hi Hal,<br>
><br>
> If you think some other design is more appropriate, I'd be happy to<br>
> update the diff.<br>
><br>
><br>
><br>
> 2014-06-25 4:34 GMT-07:00 Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> > :<br>
><br>
><br>
> Hi deadal,<br>
><br>
> + // When facing an Aggregate, we have to find the element to fetch<br>
> from.<br>
> + while (SrcVal->getType()->isAggregateType()) {<br>
> + if (StructType *ST = dyn_cast<StructType>(SrcVal->getType())) {<br>
> + const StructLayout &Layout = *DL.getStructLayout(ST);<br>
> + unsigned i = 0;<br>
> + for (unsigned cd = 0, e = ST->getNumElements(); cd != e; ++cd) {<br>
> + if (Layout.getElementOffset(cd) > Offset) break;<br>
> + i = cd;<br>
> + }<br>
> +<br>
> + SrcVal = Builder.CreateExtractValue(SrcVal, i);<br>
> + Offset -= Layout.getElementOffset(i);<br>
><br>
> What happens if Offset goes beyond the end of the struct?<br>
><br>
><br>
><br>
> I was trusting the alias analysis to not feed me this, but that I<br>
> true that i should add a check, let me provide an updated diff to<br>
> make sure we don't go off road on that one. However, I'm not sure<br>
> what a good test case for this would look like.<br>
><br>
><br>
> @@ -1919,6 +1938,11 @@ bool GVN::processLoad(LoadInst *L) {<br>
> if (DL) {<br>
> StoredVal = CoerceAvailableValueToLoadType(StoredVal, L->getType(),<br>
> L, *DL);<br>
> + if (!StoredVal) {<br>
> + // In case of aggregate store, we may have to fallback to<br>
> GetStoreValueForLoad.<br>
> + StoredVal = GetStoreValueForLoad(DepSI->getValueOperand(), 0,<br>
> L->getType(), L, *DL);<br>
> + }<br>
> +<br>
> if (!StoredVal)<br>
> return false;<br>
><br>
> I'm a bit concerned here that we're changing the logic for things<br>
> other than the aggregate case. CoerceAvailableValueToLoadType can<br>
> return false for all sorts of reasons.<br>
><br>
><br>
><br>
> As said, if you think there is a better way to do this, i'd be happy<br>
> to update the diff.<br>
<br>
</div></div>I think the first step is to understand the other reasons why CoerceAvailableValueToLoadType returns null, and see if this new behavior is generally desirable. Do you have any thoughts on that?<br>
<span><font color="#888888"><br>
 -Hal<br>
</font></span><div><div><br>
><br>
><br>
><br>
> Owen, while I'm also concerned about code coverage here, I see no<br>
> technical reason to discourage this: I don't see why we should force<br>
> all frontends to adopt the Clang method, which involves lots of<br>
> bitcasts and memcpys. This is a much larger semantic restriction<br>
> than "put your allocas in the entry block."<br>
><br>
> Chandler, could you please look at this? Is this something that SROA<br>
> should be dealing with first?<br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> ><br>
> > To: "Owen Anderson" < <a href="mailto:owen@apple.com" target="_blank">owen@apple.com</a> ><br>
> > Cc: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > Sent: Tuesday, June 24, 2014 6:02:14 PM<br>
> > Subject: Re: Optimize load from aggregate stores<br>
> ><br>
> ><br>
> ><br>
> > ping ?<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > 2014-06-13 13:30 GMT-07:00 deadal nix < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> > :<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > 2014-06-12 9:25 GMT-07:00 Owen Anderson < <a href="mailto:owen@apple.com" target="_blank">owen@apple.com</a> > :<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Jun 12, 2014, at 9:02 AM, Philip Reames <<br>
> > <a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a> > wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On 06/12/2014 12:28 AM, Owen Anderson wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Jun 11, 2014, at 11:29 PM, deadal nix < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> > It is gonna improve the situation quite a lot for all frontend that<br>
> > use aggregate loads (arguably, that is a bad practice, but that no<br>
> > reason to stab people in the back when they do it anyway).<br>
> ><br>
> ><br>
> > I’m not sure I agree with that statement. If we don’t think they<br>
> > should be used, not optimizing them is a good way to discourage<br>
> > that. More generally, I’m concerned about how we will ever get good<br>
> > test coverage of this code path, since we don’t have any extant<br>
> > front ends that hit it. I'm joining this discussion late, but a)<br>
> > why<br>
> > are aggregate loads bad practice? Loading something like a small<br>
> > struct from memory with a single load seems reasonable.<br>
> ><br>
> ><br>
> > They are not well supported through the code generator, and<br>
> > introducing them would add a lot of complexity. There are better<br>
> > solutions already in use. The only encouraged use case for first<br>
> > class structs is as multiple return values.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > Refusing a patches to improve support because something is not well<br>
> > supported is the best way to ensure they are never well supported.<br>
> ><br>
> ><br>
> ><br>
> ><br>
><br>
><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div></div></div><br></div>
</blockquote></div><br></div>