<div dir="ltr">After looking at the benchmark motivating the test, I find it is much harder for codegenprepare to catch all the shrinking opportunities compared with instcombine, because of two things:<div><br></div><div>* loadpre may merge the original load in a shrinking pattern with other load in a very early position, and there maybe other stores in between. The safety check for the codegenprepare now simply scan mayWriteToMemory insns between load and store in the candidate shrinking pattern, and it will fail because of those stores in between. </div><div><br></div><div>* we may see the following pattern. a.f1 is a bitfield. Ideally, we should do the shrinking for all the three stores. But codegenprepare works in top-down order. After the first store is shrinked, it is hard for the second and the third to know %bf.set is the same as the value loaded from %0. If we do this in instcombine, it is much easier because before load/store elimination, %bf.set will be loaded from %0 respecitively in if.then9 and in if.end41. </div><div><br></div><div>entry:</div><div>  %0 = bitcast %class.B* %this to i64*</div><div>  %bf.load = load i64, i64* %0, align 8</div><div><div>  %dec = add i64 %bf.load, 65535</div><div>  %bf.value = and i64 %dec, 65535</div><div>  %bf.clear5 = and i64 %bf.load, -65536</div><div>  %bf.set = or i64 %bf.value, %bf.clear5</div><div>  store i64 %bf.set, i64* %0, align 8</div></div><div>  ...</div><div>if.then9:</div><div><div>  %inc79 = add i64 %bf.set, 281474976710656       // we hope to know %bf.set is the same as load i64, i64* %0, align 8.</div><div>  %bf.shl = and i64 %inc79, 71776119061217280</div><div>  %bf.clear18 = and i64 %bf.set, -71776119061217281</div><div>  %bf.set19 = or i64 %bf.shl, %bf.clear18</div><div>  store i64 %bf.set19, i64* %0, align 8</div></div><div>  ...</div><div>if.end41:</div><div><div>  %inc4578 = add i64 %bf.set, 65536                       // we hope to know %bf.set is the same as load i64, i64* %0, align 8.</div><div>  %bf.shl48 = and i64 %inc4578, 4294901760</div><div>  %bf.clear49 = and i64 %bf.set, -4294901761</div><div>  %bf.set50 = or i64 %bf.shl48, %bf.clear49</div><div>  store i64 %bf.set50, i64* %0, align 8</div></div><div><br></div><div>There are two types of shrinking in the patch now: 1. shrinking without orginal load. 2. shrinking with original load -- shrinking illegal type to legal type (handling test from <span style="font-size:19.2px">test/CodeGen/ARM/illegal-</span><wbr style="font-size:19.2px"><span style="font-size:19.2px">bitfield-loadstore.ll). Since the second type shrinking may need TargetLowering information and better stay in CodeGenPrepare, I intend to split the first type shrinking and put it into instcombine, so that part of shrinking can be more useful than staying in CodeGenPrepare. </span></div><div><br></div><div>Thanks,</div><div>Wei.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Mar 4, 2017 at 12:49 PM, Wei Mi via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">wmi added a comment.<br>
<br>
Although I did't find regression in internal benchmarks testing, I still moved the transformation to codegenprepare because we want to use TargetLowering information to decide how to shrink in some cases.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D30416" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D30416</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>