<div class="gmail_quote">On Mon, Mar 19, 2012 at 9:10 PM, Chris Lattner <span dir="ltr"><<a href="mailto:sabre@nondot.org">sabre@nondot.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div id=":7ia">- The instsimplify generalizations look fine to me, please commit them as a separate patch though.<br></div></blockquote><div><br></div><div>Cool. Turns out almost none of them were needed. See below. =] I've committed the one that I did need in </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":7ia">
- I like the patch to Inliner.cpp :-)<br>
- The rest of the patch looks good, one typeo: "entriy".<br>
<br>
As an effort to save some compile time, you could skip calling SimplifyInstOperands if none of the operands in FoldMappedInstruction were remapped.  If the code going into the inliner is already simplified (I think it should be due to instcombine running early?) then the non-remapped case shouldn't be simplifiable anyway.</div>
</blockquote></div><br><div>So, Duncan had a great idea, which took me back to the first way I tried to do this -- just clone the instruction, and then run the simplify routine on the instruction. I worried about whether this would be more costly, but I actually think it's *cheaper* here. We were already going to do the remapping of every operand to the instruction, we just did it in a late pass over the instructions. That isn't terribly efficient to start with, it forces us to walk the instruction list twice rather than once. Worse, we do the exact same remapping of each operand in both places, taking two trips through the value remapper in the common case where we can't simplify the instruction away.</div>
<div><br></div><div>So I changed the code to do the operand remapping eagerly (for non-PHI nodes) and to directly simplify the cloned instructions. Much simpler now I think. =] I've attached the new patch.</div><div><br>
</div><div>There are a couple of things I want to do as follow-ups: simplify PHINodes to catch cases where all phi operands end up equal. This will in-turn want to recursively apply that simplification to users of the PHINodes. This has to happen in the lazy PHINode resolution pass, so I wanted to do it in a follow-up patch.</div>
<div><br></div><div>Finally, this removes the Fold... method because it contained a single function call to SimplifyInstruction by the time I was done. I'd be interested in doing some general tidying of this code, preferably after I stop experimenting with features:</div>
<div>1) condense the pile-o-arguments</div><div>2) maybe split some of the code up since the original split didn't work</div><div>3) maybe de-class-ify this since we're not really saving any argument passing now</div>
<div><br></div><div>#2 and #3 are potentially exclusive, i wont know until i go into janitor mode. Let me know if this would be good, and whether I should just commit these cleanups (and the PHINode optimization) directly, or send 'em out to the mailing list.</div>