<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 4, 2015 at 10:35 PM, Philip Reames <span dir="ltr"><<a 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=""><div>On 03/03/2015 08:04 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote 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 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>In <a 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></span>
Ok. I think you're misinterpreting my comments, but let's debate
that over a beer <span class="aBn" tabindex="0"><span class="aQJ">tomorrow</span></span>, 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 <span class="aBn" tabindex="0"><span class="aQJ">tomorrow</span></span>. <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></blockquote><div><br></div><div>I mean, happy to debate, but I'm not even sure we're on a different page here. The stages you describe seem completely good to me. Notably, making the changes incremental and small without backtracking in terms of design or adding and then removing unneeded infrastructure. But anyways...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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><span class="">
<br>
<br>
<br>
<blockquote 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></span>
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></blockquote><div><br></div><div>Ah, of course. I had forgotten that this all started with GVN, which is why I never even considered this email thread when searching...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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.</div></blockquote><div><br></div><div>Yes, that was my thought.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"> 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.</div></blockquote><div><br></div><div>I keep looking for this too as some of the context seems to have slipped my mind.... But I'm slowly teasing it back together.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"> I
believe that's what led directly to the current patch. If you're
okay with getting this in InstCombine, let's do so. </div></blockquote></div><br>Indeed. So there are issues with splitting up an FCA store into multiple smaller stores. They are the same issues as with all other store splitting -- it loses the fact that a store was valid in the memory model (IE, doesn't race). But this seems a very minor concern (to me) for FCAs. We should make sure it doesn't lose other information that is significant (your comment about dereferencability is what I'm thinking about) but otherwise I don't see any specific problems here.</div></div>