This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner][X86][AArch64] (x - C) + y -> (x + y) - C fold
ClosedPublic

Authored by lebedev.ri on May 22 2019, 8:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.May 22 2019, 8:10 AM

Its rather annoying that the DAGCombiner::visitSUB limits the (sub x, c) -> (add x, -c) fold to non-vectors

Its rather annoying that the DAGCombiner::visitSUB limits the (sub x, c) -> (add x, -c) fold to non-vectors

I suppose i could take a look at that, but i'd like to cram out the remaining patches for sink-addsub-of-const.ll.
And the follow-ups - preservation of neg, creation of neg (no new ISD opcode), thus fixing PR41952.

RKSimon accepted this revision.May 26 2019, 6:16 AM

LGTM with one minor

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2457 ↗(On Diff #200751)

Add a comment explaining that this is necessary because SUB(X,C) -> ADD(X,-C) doesn't work for vectors.

This revision is now accepted and ready to land.May 26 2019, 6:16 AM
lebedev.ri marked an inline comment as done.

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.May 28 2019, 12:04 PM

One of the patches seems to have introduced a hang in test-suite, reverted.

This revision is now accepted and ready to land.May 28 2019, 12:04 PM

This particular patch appears to be the culprit, it's parent patches are fine.

This particular patch appears to be the culprit, it's parent patches are fine.

And reduced test-suite/MultiSource/Benchmarks/TSVC/ControlFlow-dbl/tsc.c to

; ModuleID = 'bugpoint-reduced-simplified.bc'
source_filename = "input.c"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define dso_local <4 x i32> @b(<4 x i64> %arg) local_unnamed_addr #0 {
  %t0 = add <4 x i64> %arg, <i64 8, i64 8, i64 8, i64 8>
  %t1 = trunc <4 x i64> %t0 to <4 x i32>
  %t2 = add <4 x i32> %t1, <i32 1, i32 1, i32 1, i32 1>
  %t3 = and <4 x i32> %t2, <i32 3, i32 3, i32 3, i32 3>
  ret <4 x i32> %t3
}

attributes #0 = { "use-soft-float"="false" }

!llvm.ident = !{!0}

!0 = !{!"clang version 9.0.0 (trunk 362014) (llvm/trunk 362022)"}

Hmm yeah, i should have seen it coming.
These patches will expose a lot of missing constant folds.
And by expose i mean it will stumble into cases like

; ((%arg0 + 8) - (-1))
%t0 = add %arg0, 8 ; 8 is constant, so hoist binop
%t1 = sub %t0, -1

and will change that to

; ((%arg0 - (-1)) + 8)
%t0 = sub %arg0, -1 ; -1 is constant, so hoist binop
%t1 = add %t0, 8

and then the complementary fold will undo that, if we fail to constant-fold inbetween.

This revision was automatically updated to reflect the committed changes.