[PATCH] D130839: [DAG] FoldConstantArithmetic - add initial support for undef elements in bitcasted binop constant folding

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm accepted this revision.
xiangzhangllvm added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5589
+          if (!(IsUndef1 || IsUndef2))
+            Fold = FoldValue(Opcode, C1, C2);
+          else if (Opcode == ISD::AND || Opcode == ISD::MUL)
----------------
efriedma wrote:
> xiangzhangllvm wrote:
> > RKSimon wrote:
> > > xiangzhangllvm wrote:
> > > > RKSimon wrote:
> > > > > xiangzhangllvm wrote:
> > > > > > Do we need to make sure the C1 C2 has no other uses ?
> > > > > I don't think we have any one use limits on constant folding?
> > > > Yes, sorry my mistake at first.
> > > > take following case for example:
> > > > ```
> > > > A = and(x, undef)
> > > > C = A op B
> > > > ```
> > > > If we directly let "A = 0" (I think A=undef before), seems it may break the undef propagation in other use. for example C when "op" can do undef propagation (but not constant zero propagation).
> > > ```
> > > A = and(x, undef)
> > > C = A op B
> > > ```
> > > A can't (or shouldn't) propagate undef: https://alive2.llvm.org/ce/z/0EB60y - we almost certainly have cases of that in code, which need addressing
> > > 
> > Yes, thanks for your cases! it really not propagate.
> > 
> > But sorry for I am still some puzzle here:
> > Do you know why we can't (or shouldn't) propagate undef ? 
> > 
> > To me, I can't see the real difference between
> > ```
> >  A = undef 
> > ```
> >  and 
> > ```
> >  A = and(x, undef)
> > ```
> and(0, undef) is not undef; it's zero.  By contrast, and(0, poison) is poison.  This is the fundamental difference between undef and poison. See https://llvm.org/docs/LangRef.html#undefined-values .
Very clear now, thanks very much for your explain!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130839