[PATCH] D119654: [SDAG] enable binop identity constant folds for add/sub

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3336
+  if (N1->getOpcode() == ISD::FREEZE && N0.hasOneUse())
+    N1 = N1->getOperand(0);
+
----------------
RKSimon wrote:
> Is there anyway we can reduce the scope of this initially, its likely that losing freeze like this might have other effects - if we're just after the (sub x, x) -> 0 fold, maybe create a peekThroughFreeze helper:
> 
> if (peekThroughFreeze(N0) == peekThroughFreeze(N1))
>     return tryFoldToZero(DL, TLI, VT, DAG, LegalOperations);
Good suggestion. I'll apply your idea. Thanks.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2148
+      }
+    }
     return false;
----------------
RKSimon wrote:
> LuoYuanke wrote:
> > xbolva00 wrote:
> > > You can use Constantexpr::getBinOpIdentity to check if identity constant.
> > > 
> > > 
> > Sorry, this is SDNode and it seems there is no getBinOpIdentity() API. 
> > BTW, there is some regression on this patch. 
> > 1. When the select can be combined with its operands, we don't need to invert the select folding. See below example.
> > 
> > ```
> > ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512vbmi2,+avx512vl --show-mc-encoding | FileCheck %s --check-prefixes=CHECK,X64
> > define <16 x i16> @test_int_x86_avx512_mask_vpshldv_w_256(<16 x i16> %x0, <16 x i16> %x1, <16 x i16>* %x2p, <16 x i16> %x4, i16 %x3) {
> >   %x2 = load <16 x i16>, <16 x i16>* %x2p
> >   %1 = call <16 x i16> @llvm.fshl.v16i16(<16 x i16> %x0, <16 x i16> %x1, <16 x i16> %x2)
> >   %2 = bitcast i16 %x3 to <16 x i1>
> >   %3 = select <16 x i1> %2, <16 x i16> %1, <16 x i16> %x0
> >   %4 = call <16 x i16> @llvm.fshl.v16i16(<16 x i16> %x0, <16 x i16> %x1, <16 x i16> %x4)
> >   %5 = bitcast i16 %x3 to <16 x i1>
> >   %6 = select <16 x i1> %5, <16 x i16> %4, <16 x i16> zeroinitializer
> >   %res3 = add <16 x i16> %3, %6
> >   ret <16 x i16> %res3
> > }
> > 
> > declare <16 x i16> @llvm.fshl.v16i16(<16 x i16> %x0, <16 x i16> %x1, <16 x i16> %x4)
> > ```
> > 2. The freeze node seems to prevent the sub combine for below case.
> > 
> > ```
> > define <4 x i32> @test_srem_allones(<4 x i32> %X) nounwind {
> >   %srem = srem <4 x i32> %X, <i32 4294967295, i32 4294967295, i32 4294967295, i32 4294967295>
> >   %cmp = icmp eq <4 x i32> %srem, <i32 0, i32 0, i32 0, i32 0>
> >   %ret = zext <4 x i1> %cmp to <4 x i32>
> >   ret <4 x i32> %ret
> > }
> > ```
> > You can use Constantexpr::getBinOpIdentity to check if identity constant.
> 
> We already have SelectionDAG::getNeutralElement - I wonder if adding a SelectionDAG::isNeutralElement helper sibling would be useful?
I prefer to inverting the operation one by one, so that the patch can be small. After all the operators are inverted, we can refactor the code by using isNeutralElement() and getNeutralElement(). What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119654