Page MenuHomePhabricator

[DAGCombine][X86][AArch64][MIPS][LANAI] (C - x) - y -> C - (x + y) fold (PR41952)
ClosedPublic

Authored by lebedev.ri on Jun 1 2019, 5:50 AM.

Details

Summary

This *might* be the last fold for sink-addsub-of-const.ll, but i'm not sure yet.

As far as i can tell, there are no regressions here (ignoring x86-32),
all changes are either good or neutral.

This, almost surprisingly to me, fixes the motivational tests (in shift-amount-mod.ll)
@reg32_lshr_by_sub_from_negated from PR41952.

https://rise4fun.com/Alive/vMd3

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 1 2019, 5:50 AM
RKSimon edited reviewers, added: efriedma; removed: eli.friedman.Jun 1 2019, 10:58 AM
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3017

Do we gain anything from the general case here? This seems to cause a small increase in register pressure in many cases - limit to the (0 - x) - y negation case only?

lebedev.ri added inline comments.Jun 1 2019, 11:19 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3017

Could you please be more specific?
The only regressions i'm seeing are 32-bit @reg64_lshr_by_sub_from_negated*
The obvious positives are all these newly-formed neg
https://godbolt.org/z/JK0ldm <- which is kinda the endgoal here.

If we limit this to only hoist neg (and this will be the first such restriction
in all of these patches), then naturally, the @reg64_lshr_by_sub_from_negated*
pattern will no longer produce neg, since the sub 64, %x will not be sunk.

That can still be fixed, if we transform C-X to neg(x)+C or so in DAGCombine,
I was actually going to look into that, but it looked to not be necessary
with this patch in it's current form.

RKSimon accepted this revision.Jun 2 2019, 4:42 AM

LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3017

Yes I think your right - sorry I was being over cautious!

This revision is now accepted and ready to land.Jun 2 2019, 4:42 AM
lebedev.ri marked 4 inline comments as done.Jun 2 2019, 11:41 AM

Thank you for the review!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3017

No problem, thank you for confirming my understanding here!

This revision was automatically updated to reflect the committed changes.