<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 2, 2015 at 4:11 PM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":4q7" class="a3s" style="overflow:hidden">So, basically you operate on an aggregate that contains two i8. In one case it is copied using memcpy and the other it is copied using load/store.<br>
What triggers your “bug” is that SROA does not handle these two copies the same way.<br>
The memcpy version is turned into two independent i8 stores:<br>
<span class=""><br>
  %v.sroa.0.0.dst.sroa_idx = getelementptr inbounds { i8, i8 }* %0, i64 0, i32 0<br>
  store i8 1, i8* %v.sroa.0.0.dst.sroa_idx, align 1<br>
  %v.sroa.2.0.dst.sroa_idx = getelementptr inbounds { i8, i8 }* %0, i64 0, i32 1<br>
  store i8 2, i8* %v.sroa.2.0.dst.sroa_idx, align 1<br>
<br>
</span>while the load/store version does not change the store:<br>
<br>
  %tmp.fca.0.insert = insertvalue { i8, i8 } undef, i8 1, 0<br>
  %tmp.fca.1.insert = insertvalue { i8, i8 } %tmp.fca.0.insert, i8 2, 1<br>
  store { i8, i8 } %tmp.fca.1.insert, { i8, i8 }* %0<br>
<br>
<br>
Now I am not sure if SROA shouldn’t produce the same result for the two inputs in this case? (i.e. splitting the store to the aggregate in store to the individual element)<br>
If not then teaching GVN about this case is probably correct.</div></blockquote></div><br>I think it's neither...</div><div class="gmail_extra"><br></div><div class="gmail_extra">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><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.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">As a side note, I don't know what Rust's ABI concerns are, but if at all possible, I would suggest moving away from FCAs in the frontend as much as possible. Everything I have seen in LLVM is that they obstruct optimization in significant ways.</div></div>