<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>