The patch looks good to me. A couple of minor comments about testing this...<div><br></div><div>Because it is a phase-ordering kind of problem, I worry a lot about regressions. Specifically, if we ever get some other pass that nukes your "silly function", will we stop testing that this works?<br>
</div><div><br></div><div>I wonder, could we add a direct test that SCEV can undrestand this pattern when InstCombine runs first? That would seem more targeted.</div><div><br></div><div>Can we add a test for the LSR usage as well? Maybe using the same pattern as above?</div>
<div><br></div><div>Finally, a nit pick on the patch:</div><div><div><br></div><div>diff --git a/test/Transforms/InstCombine/shift.ll b/test/Transforms/InstCombine/shift.ll</div><div>index 52310e3..21c0a33 100644</div><div>
--- a/test/Transforms/InstCombine/shift.ll</div><div>+++ b/test/Transforms/InstCombine/shift.ll</div></div><div>>>> snip <<<</div><div><div>+;; This is allowed only when %B is single-use.</div></div><div>
<br></div><div>Should this be "deferred to DAG combining" as with the other comments in this test file?</div><div><br></div><div>Thanks,</div><div>-Chandler</div><div><br></div>