[PATCH] D128123: [SDAG] try to replace subtract-from-constant with xor

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3766
+  // If there's no chance any bit will need to borrow from an adjacent bit:
+  // sub C, X --> xor X, C
+  if (ConstantSDNode *C0 = isConstOrConstSplat(N0)) {
----------------
barannikov88 wrote:
> barannikov88 wrote:
> > spatel wrote:
> > > barannikov88 wrote:
> > > > It might be more profitable to do this transformation in SimplifyDemandedBits. Some bits of the result might not be used downstream, in which case the check can be relaxed.
> > > > 
> > > The known-bits vs. demanded-bits transforms are two different things if I'm seeing it correctly, although there can be some overlap. Demanded bits may already be used to shrink the constants, so that could enable this transform. 
> > > 
> > > If you have an example in mind that doesn't transform as expected, would you please file an issue, so we can track the fix? If it's not working here in codegen, it may also be missing from the IR equivalent. 
> > > 
> > > I updated the IR side to match this patch with:
> > > 79bb915fb60b
> > > The known-bits vs. demanded-bits transforms are two different things if I'm seeing it correctly
> > 
> > My understanding is as follows. SimplifyDemandedBits recurses down to operands of the node propagating information about what bits are needed of them. It might help to simplify the operands. On the way back from the reucursion, the operands tell the node which bits of them are known so that the node itself can be simplified. I could be wrong.
> > I noticed that SimplifyDemandedBits is mainly used in DAGCombiner, mostly as a last resort if no other transformation could be applied. For some reason this function is not called for ISD::SUB.
> > 
> > > in which case the check can be relaxed.
> > 
> > On the second thought, it does not look so simple. Suppose we have two 2-bit operands:
> > ```
> > C = 0b10
> > X = 0b1?
> > ```
> > and we're demanding the 1-th bit (counting from 0) from the SUB. We still have to check for possible overflow in 0-th bit.
> > 
> By the way, a more general check would be to ignore borrow from the bit after the MSB. E.g.
> `(~X.KnownZero).isSubsetOf(C | SignMask)`
> or
> `(~(X.KnownZero | SignMask)).isSubsetOf(C)`
> or, which is probably a bit cleaner,
> `(C - ~X.KnownZero) == (C ^ ~X.KnownZero)`
> if I'm not mistaken.
> This, however, does not affect any existing tests. At least not in backend.
> 
Yes, that looks like a good enhancement. Here's a try at a general proof and specific example for a regression test:
https://alive2.llvm.org/ce/z/KABK4Z


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128123