<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 03/03/2015 08:04 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0Kja6bfzjAS4essNtwL7DSKYBZqvzOU6oHuRSvrfL=Dkmg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Mar 3, 2015 at 9:24 AM,
Philip Reames <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span
class="">In <a moz-do-not-send="true"
href="http://reviews.llvm.org/D7780#133316"
target="_blank">http://reviews.llvm.org/D7780#133316</a>,
@chandlerc wrote:<br>
<br>
> My primary question is why this isn't just an
instcombine, using insertvalue / extractvalue to
reconnect the aggregates?<br>
<br>
<br>
</span>@chandlerc - Because we talked about that last time
and it was rejected for some reason I don't really
remember. I would strongly prefer we land this in
something close to it's current form and then iterate in
tree. This has topic has been stalled on census for
months. Honestly, I sorta of agree with you, but let's
revisit that separately.</blockquote>
</div>
<br>
Absolutely not. I do *not* think we should add a pass if there
are multiple people who don't think it belongs as a pass.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">If we keep losing the context of why
this needs to be a pass, then we need to fix that problem
rather than just "iterate in tree" to hide the problem. It
won't get fixed otherwise, we'll just end up with bad code in
the tree.</div>
</div>
</blockquote>
Ok. I think you're misinterpreting my comments, but let's debate
that over a beer tomorrow, not here. It's too easy to talk past
each other in email. I feel like you're letting perfection be the
enemy of the good here, but again, over a beer tomorrow. <br>
<br>
My "ideal design" for the current patch *only* would be to move the
handling to instcombine. Specifically, we should unwrap a load or a
store of an FCA when that FCA contains exactly one type and the size
of that type covers the entire size of the FCA. (To be utterly
clear, this code would involve no recursion!)<br>
<br>
After that has landed, I would likely to see instcombine extended to
handle FCAs with multiple fields. I'm a bit unsure about this given
that it appears to loose dereferenceability information, but given
previously expressed desire to not change GVN to support FCAs, this
seems like a workable approach.<br>
<br>
(As a reminder, I am not personally using FCAs in any form. My only
motivation for getting involved here is that we have had *multiple*
bug reports on this topic with patches from interested parties and
we have made *no* progress on resolving the underling issue. I see
that as a problem.) <br>
<br>
<br>
<br>
<blockquote
cite="mid:CAGCO0Kja6bfzjAS4essNtwL7DSKYBZqvzOU6oHuRSvrfL=Dkmg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">Ok, so what I recall about this is that
the problem was that these were not single-instruction
replacements because there was the desire to not introduce
insertvalue / extractvalue? Am I on the right path or not?</div>
</div>
</blockquote>
Here's the quote from you in an earlier email thread titled "Add
basic support for removal of load that are fed by a store of an
aggregate":<br>
<br>
"The store that remains has nothing to do with an alloca. It is just
a store off to wild memory as an FCA. SROA shouldn't be touching it,
and I don't think we want to try to teach the entire optimizer about
FCAs.
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">You're teaching GVN about FCAs in this
patch, but we also reason about store-to-load forwarding in *many*
other places. I don't think it is really feasible to teach every
part of LLVM this.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">The alternative is to define the canonical
form as extracting the values from the FCA and storing them
individually. SROA does this for loads and stores into allocas as
a matter of correctness, but we could also teach instcombine to do
this for all FCA loads and stores as a matter of canonical form
and optimization. I think that is probably the right direction
long term, but I'm a little scared of the down-stream
rammifications.</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">The patch is trivial, and the code is
already in SROA. Let me factor it out and I can post a patch to
see if it also solves your problems. But I want to spend some time
looking at what knock-on effects this has and whether they are
reasonable."<br>
-- end quote<br>
<br>
If I'm reading you right here, you were essentially asking for an
instcombine which replaced the FCA store with two i8 stores of the
individual components. In yet another thread, which I can't seem
to find now, you said something about not wanting this (or maybe
it was the related memcpy thing? Not sure) in instcombine. I
believe that's what led directly to the current patch. If you're
okay with getting this in InstCombine, let's do so. <br>
<br>
Philip<br>
</div>
<br>
</body>
</html>