[PATCH] D62341: [DAGCombine][X86][AArch64][AMDGPU][MIPS][PPC] (sub x, c) -> (add x, -c) vector edition.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

In D62341#1515637 <https://reviews.llvm.org/D62341#1515637>, @atanasyan wrote:

> I do not think it's good for MIPS because this patch replace one `subvi` instruction by pair of `ldi` and `addv`.


Yep, i guessed as much in the patch description.

> Are you going to fix such regressions?

I have zero clues about mips/etc. I'll certainly take a look, but i just don't know.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2819
+  // fold (sub x, c) -> (add x, (sub 0, c))
+  if (isConstantOrConstantVector(N1, /* NoOpaques */ true)) {
+    SDValue NegC = DAG.FoldConstantArithmetic(
----------------
RKSimon wrote:
> Do we need a (!VT.isVector() || N1.hasOneUse()) limit?
Hmm, i'm not a fan, and let me explain why.
There are three paths, as i see it:
* Without this patch - keep `sub` for vectors.
  1. Bad - pattern duplication 
  2. Good - it is immediately obvious when the `sub` fold is missing, 
  3. Neutral - we do nothing to constant pool
* This patch as-is - always transform all `sub`, with no use-checks.
  1. Better - no pattern duplication
  2. Unknown - this has //some// effect on constant pool. 
* This patch, but limit vector transform to one-use constants
  1. Bad - the pattern duplication is still there, or Worse - we suddenly loose folds if the constant is not one-use.
  3. Worse - unless one knows about that one-use limitation, the missing fold won't even be noticed

So ignoring the constant pool issues, the one-use limit is the worst path
of them all, even worse than what we have now. Am i missing some other path?

As an alternative, i can suggest some transform to analyze all the vector `add %x, C`,
all the existing constant-pool entries (==all constant vectors, not just those used by `add`),
and try to flip some of those `add` to `sub` if that allows to kill some pool entires.
The same could be done with locality in mind (within one BB).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62341/new/

https://reviews.llvm.org/D62341