[PATCH] D62223: [DAGCombiner][X86][AArch64][AMDGPU] (x + C) - y -> (x - y) + C fold

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Do x86 test changes look good?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2926
+  // Hoist one-use addition by constant:  (x + C) - y  ->  (x - y) + C
+  if (N0.hasOneUse() && N0.getOpcode() == ISD::ADD &&
+      isConstantOrConstantVector(N0.getOperand(1))) {
----------------
bjope wrote:
> lebedev.ri wrote:
> > bjope wrote:
> > > Not sure if it is super important, or common, in reality. But maybe this should be trigger for an add-like-or as well? 
> > > 
> > > An ADD with no common bits set in the operands is canonicalized into OR by DAGCombiner, so there could be lots of OR:s out there that really are ADD:s.
> > Yes, correct observation. I did think about it when writing,
> > but i did not see any easy way to do that here.
> > Any suggestions?
> We could introduce some helper to make it easier to match an "add-like" node. However, checking "noCommonBitsSet" to detect if an OR is add-like currently involves ValueTracking, so it is not a cheap operation. We probably want to see that we get some payback if doing that, and not just wasting time.
> 
> Since we already match on ISD::ADD in several places above, it might have been too much to ask for in this patch. So I think we can save it for later.
> 
> Since we already match on ISD::ADD in several places above, it might have been too much to ask for in this patch. So I think we can save it for later.

I agree. Thank you for taking a look.


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.s.barrier.ll:6
 ; RUN: llc -march=amdgcn -mcpu=gfx900 -mattr=+auto-waitcnt-before-barrier -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN,GFX9,VARIANT3 %s
 
 define amdgpu_kernel void @test_barrier(i32 addrspace(1)* %out, i32 %size) #0 {
----------------
lebedev.ri wrote:
> @arsenm I //believe// D62263 recovers (and improves upon?) from changes here, somewhat, PTAL.
> 
> 
I've verified that with that patch, the original test (with non-auto-generated check lines) passes, so i'm going to assume this is ok.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62223