<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/04/2015 10:51 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KiTpg+uWHy2mpcQ36_tVdTa8v_GhiSbin5zcoWJ2pOXxw@mail.gmail.com"
type="cite">
<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 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="">
<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
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>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>
</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>
</div>
</blockquote>
I don't think we are in disagreement here. I didn't mean to imply we
were beyond the initial "iterate in tree" vs "get it right the first
time" part. <br>
<blockquote
cite="mid:CAGCO0KiTpg+uWHy2mpcQ36_tVdTa8v_GhiSbin5zcoWJ2pOXxw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</blockquote>
Ok, I think we're in agreement here. Let's move the "interesting"
bits of the proposed patch over to being a patch for instcombine. I
agree that the load legality and dereferenceability issues are
secondary. We probably want to document that with a comment, but
that's not even the first patch, so let's defer it. <br>
<br>
deadalnix - Do you want to do this step? I could understand your
being a bit frustrated at this point. If not, I'm happy to handle
getting this ported over and reviewed. Just let me know. I really
want to see this land. <br>
<br>
Philip<br>
<br>
</body>
</html>