<div dir="ltr"><div class="gmail_extra"><div><div class="gmail_signature">On Thu, Feb 9, 2017 at 2:11 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Thu, Feb 9, 2017 at 2:09 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>> wrote:<br>
> On Thu, Feb 9, 2017 at 2:01 PM, Davide Italiano <<a href="mailto:davide@freebsd.org">davide@freebsd.org</a>> wrote:<br>
>><br>
>> On Thu, Feb 9, 2017 at 1:46 PM, Michael J. Spencer via llvm-commits<br>
>> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> > Author: mspencer<br>
>> > Date: Thu Feb  9 15:46:49 2017<br>
>> > New Revision: 294632<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=294632&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=294632&view=rev</a><br>
>> > Log:<br>
>> > [LoadCombine] Fix combining of loads which span an aliasing store.<br>
>> ><br>
>> > Fixes PR31517<br>
>> ><br>
>> > Differential Revision: <a href="https://reviews.llvm.org/D28922" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28922</a><br>
>> ><br>
>> > Modified:<br>
>> >     llvm/trunk/lib/Transforms/<wbr>Scalar/LoadCombine.cpp<br>
>> >     llvm/trunk/test/Transforms/<wbr>LoadCombine/load-combine-aa.ll<br>
>> ><br>
>> > Modified: llvm/trunk/lib/Transforms/<wbr>Scalar/LoadCombine.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp?rev=294632&r1=294631&r2=294632&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Scalar/LoadCombine.<wbr>cpp?rev=294632&r1=294631&r2=<wbr>294632&view=diff</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- llvm/trunk/lib/Transforms/<wbr>Scalar/LoadCombine.cpp (original)<br>
>> > +++ llvm/trunk/lib/Transforms/<wbr>Scalar/LoadCombine.cpp Thu Feb  9 15:46:49<br>
>> > 2017<br>
>> > @@ -245,13 +245,17 @@ bool LoadCombine::runOnBasicBlock(<wbr>BasicB<br>
>> >    bool Combined = false;<br>
>> >    unsigned Index = 0;<br>
>> >    for (auto &I : BB) {<br>
>> > -    if (I.mayThrow() || (I.mayWriteToMemory() &&<br>
>> > AST.containsUnknown(&I))) {<br>
>> > +    if (I.mayThrow() || AST.containsUnknown(&I)) {<br>
>> >        if (combineLoads(LoadMap))<br>
>> >          Combined = true;<br>
>> >        LoadMap.clear();<br>
>> >        AST.clear();<br>
>> >        continue;<br>
>> >      }<br>
>> > +    if (I.mayWriteToMemory()) {<br>
>> > +      AST.add(&I);<br>
>> > +      continue;<br>
>> > +    }<br>
>> >      LoadInst *LI = dyn_cast<LoadInst>(&I);<br>
>> >      if (!LI)<br>
>> >        continue;<br>
>> ><br>
>> > Modified: llvm/trunk/test/Transforms/<wbr>LoadCombine/load-combine-aa.ll<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll?rev=294632&r1=294631&r2=294632&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/LoadCombine/load-<wbr>combine-aa.ll?rev=294632&r1=<wbr>294631&r2=294632&view=diff</a><br>
>> ><br>
>> > ==============================<wbr>==============================<wbr>==================<br>
>> > --- llvm/trunk/test/Transforms/<wbr>LoadCombine/load-combine-aa.ll (original)<br>
>> > +++ llvm/trunk/test/Transforms/<wbr>LoadCombine/load-combine-aa.ll Thu Feb  9<br>
>> > 15:46:49 2017<br>
>> > @@ -37,3 +37,24 @@ define i64 @test2(i32* nocapture readonl<br>
>> >    ret i64 %add<br>
>> >  }<br>
>> ><br>
>> > +%rec11 = type { i16, i16, i16 }<br>
>> > +@str = global %rec11 { i16 1, i16 2, i16 3 }<br>
>> > +<br>
>> > +; PR31517 - Check that loads which span an aliasing store are not<br>
>> > combined.<br>
>> > +define i16 @test3() {<br>
>> > +; CHECK-LABEL: @test3<br>
>> > +<br>
>> > +; CHECK: load i16, i16*<br>
>> > +; CHECK: store i16<br>
>> > +; CHECK: ret i16<br>
>> > +<br>
>> > +  %_tmp9 = getelementptr %rec11, %rec11* @str, i16 0, i32 1<br>
>> > +  %_tmp10 = load i16, i16* %_tmp9<br>
>> > +  %_tmp12 = getelementptr %rec11, %rec11* @str, i16 0, i32 0<br>
>> > +  store i16 %_tmp10, i16* %_tmp12<br>
>> > +  %_tmp13 = getelementptr %rec11, %rec11* @str, i16 0, i32 0<br>
>> > +  %_tmp14 = load i16, i16* %_tmp13<br>
>> > +  %_tmp15 = icmp eq i16 %_tmp14, 3<br>
>> > +  %_tmp16 = select i1 %_tmp15, i16 1, i16 0<br>
>> > +  ret i16 %_tmp16<br>
>> > +}<br>
>> ><br>
>> ><br>
>><br>
>> I think it's a good idea to change this tests to not rely on<br>
>> instcombine anymore.<br>
><br>
><br>
> The IR gets pretty ugly if you do that and makes the test harder to<br>
> understand.<br>
><br>
<br>
</div></div>Hmm, but that also makes the test more fragile as we rely on<br>
instcombine doing some cleanups before loadcombine. I don't think we<br>
want that dependency. What do you think?<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div><br></div><div>It's been in tree for 2 years with no changes due to changes in instcombine. So I think it's fine.</div><div><br></div><div>- Michael Spencer </div></div></div></div>