[PATCH] D68408: [InstCombine] Unfold `sub %x, %y` -> `add (sub 0, %y), %x` IFF `%y` can be freely negated

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: spatel, nikic, efriedma.
lebedev.ri added a project: LLVM.
Herald added a subscriber: hiraditya.
lebedev.ri edited the summary of this revision.

As we have discussed previously (e.g. in D63992 <https://reviews.llvm.org/D63992> / D64090 <https://reviews.llvm.org/D64090> / PR42457 <https://bugs.llvm.org/show_bug.cgi?id=42457>), `sub` instruction
can almost be considered non-canonical. While we do convert `sub %x, C` -> `add %x, -C`,
we sparsely do that for non-constants. But we should.

Here, i propose to explode `sub %x, %y` into `add (sub 0, %y), %x` IFF we *know* that `neg` can be sinked into the `%y`
My main motivation here is the `@t7`, which is from PR42412 <https://bugs.llvm.org/show_bug.cgi?id=42412> / PR42389 <https://bugs.llvm.org/show_bug.cgi?id=42389>.

We know that `neg` is free if:

- We are negating an integral constant - https://rise4fun.com/Alive/6ploz
- The instruction in question has no users other than the negation itself. The rest of reasoning is recursive.
- It's a `select` - both hands must be negatible - https://rise4fun.com/Alive/MYge
- It's a `shl` - first operand must be negatible - https://rise4fun.com/Alive/7kA
- It's a `sub` - first operand must be negatible - https://rise4fun.com/Alive/ati7
- It's a `add` - both operands must be negatible - https://rise4fun.com/Alive/ou7
- It's a `mul` - either one of operands must be negatible - https://rise4fun.com/Alive/4tR
- (FIXME: there are likely more cases?)

Normally, InstCombine's worklist would cause the `add (sub 0, %y), %x` to be revisited first,
which would immediately get folded back into `sub %x, %y`, and we'd loop endlessly.
We want the `sub 0, %y` to be revisited first, so we have to manually mess with worklist.
This *is* unusual. Is this too ugly to live?

This does have some potential to expose missing folds and cause endless combine loops.

There's also a second part to this coin - while we currently mindlessly hoist `neg` if we can,
as it can be seen in `@dont_touch_negation_itself`'s case - there should not be a `neg` there,
https://rise4fun.com/Alive/58xqwe
We should make use of this infrastructure to muster through in the opposite direction:
if `isFreeToNegate()` says that the `neg` can be sinked, we should sink it.

(The third part of the coin is that we may want to do the same for inversion (`xor %x, -1`).)

Thoughts?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68408

Files:
  llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
  llvm/lib/Transforms/InstCombine/InstCombineInternal.h
  llvm/test/Transforms/InstCombine/sub-of-negatible.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D68408.223048.patch
Type: text/x-patch
Size: 6667 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191003/3e02a9cf/attachment.bin>