[llvm-commits] [PATCH] Defer some shl transformations from InstCombine to DAGCombine
Jakob Stoklund Olesen
stoklund at 2pi.dk
Wed Apr 18 16:48:52 PDT 2012
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
More information about the llvm-commits
mailing list