[llvm-dev] visitShiftByConstant of DAGCombiner

[Renato Golin via llvm-dev]

On 8 December 2016 at 02:34, Jojo Ma <jojo.ma at linaro.org> wrote:
> It would be profitable as well if we could enable the canonicalisation on
> it.
> sequence before this canonicalisation (ARM):
> test:
> .fnstart
> @ BB#0:                                 @ %entry
> movw r1, #65534
> and r1, r0, r1
> ubfx r0, r0, #1, #15
> add r0, r0, r1, lsr #1
> bx lr
>
> sequence after this canonicalisation:
> test:
> .fnstart
> @ BB#0:                                 @ %entry
> ubfx r0, r0, #1, #15
> add r0, r0, r0
> bx lr
>
> But when I tried to expand the condition to this case, there are lots of
> various regression fails.

Hi Jojo,

I think this canonicalisation is correct, at least in this case, so
it's quite possible that the regressions are just bad tests, or you
actually made the code better in other places that weren't expecting
it.

I'm adding some folks that have worked on the DAG combiner recently to
have a look, but it would be good to know what regressions were there
to see if they're really regressions (ie. worse code) or just
different/better code.

You may also have missed a check on your code (which would wrongly
combine constants). Can you create a review on Phab of your proposed
change?

Also, if you could list the regressions and see if they have a common
pattern (ie. "tests expecting an additional mask failed", or "the
wrong mask was applied"), it would become clearer if they are real
regressions or not.

cheers,
--renato