<div dir="ltr"><div>Hi Hal,<br><br></div>If you think some other design is more appropriate, I'd be happy to update the diff.<div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-25 4:34 GMT-07:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi deadal,<br>
<br>
+ // When facing an Aggregate, we have to find the element to fetch 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></blockquote><div><br>I was trusting the alias analysis to not feed me this, but that I true
that i should add a check, let me provide an updated diff to make sure
we don't go off road on that one. However, I'm not sure what a good test
case for this would look like.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -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 GetStoreValueForLoad.<br>
+ StoredVal = GetStoreValueForLoad(DepSI->getValueOperand(), 0, 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 other than the aggregate case. CoerceAvailableValueToLoadType can return false for all sorts of reasons.<br>
<br></blockquote><div><br>As said, if you think there is a better way to do this, i'd be happy to update the diff.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Owen, while I'm also concerned about code coverage here, I see no technical reason to discourage this: I don't see why we should force all frontends to adopt the Clang method, which involves lots of bitcasts and memcpys. This is a much larger semantic restriction than "put your allocas in the entry block."<br>
<br>
Chandler, could you please look at this? Is this something that SROA should be dealing with first?<br>
<br>
-Hal<br>
<div class=""><div class="h5"><br>
----- Original Message -----<br>
> From: "deadal nix" <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>><br>
> To: "Owen Anderson" <<a href="mailto:owen@apple.com">owen@apple.com</a>><br>
> Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">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">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">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">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">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) 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>
</div></div><div class=""><div class="h5">> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
</div></div><span class=""><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>