[llvm-commits] [PATCH] Defer some shl transformations from InstCombine to DAGCombine

[Jakob Stoklund Olesen]

On Apr 18, 2012, at 4:28 PM, Chandler Carruth <chandlerc at google.com> wrote:

> The patch looks good to me. A couple of minor comments about testing this...
> 
> 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?

I am all for more tests. The PhaseOrdering test does catch this, though (It runs opt -O3 = all standard passes):

; CHECK: @test3
; CHECK: %div = lshr i32 %a, 2
; CHECK: %add = shl nuw nsw i32 %div, 1

This tests that %add is expressed as a multiple of %div, unlike before:

  %div = lshr i32 %a, 2
  %0 = lshr i32 %a, 1
  %add = and i32 %0, 2147483646

I'll add a comment to the test.

> I wonder, could we add a direct test that SCEV can undrestand this pattern when InstCombine runs first? That would seem more targeted.

I'll try to figure out if that is possible.

> Can we add a test for the LSR usage as well? Maybe using the same pattern as above?

LSR is using SCEV, so I am not so worried about that. LSR tests are also tricky because it needs target information, so it can only be run from llc.

> Finally, a nit pick on the patch:
> 
> diff --git a/test/Transforms/InstCombine/shift.ll b/test/Transforms/InstCombine/shift.ll
> index 52310e3..21c0a33 100644
> --- a/test/Transforms/InstCombine/shift.ll
> +++ b/test/Transforms/InstCombine/shift.ll
> >>> snip <<<
> +;; This is allowed only when %B is single-use.
> 
> Should this be "deferred to DAG combining" as with the other comments in this test file?

Yes, will fix.

Thanks,
/jakob