[PATCH] D128159: [DAG] Enable scalable vectors handling in computeKnownBits

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3468
   case ISD::ZERO_EXTEND: {
     Known = computeKnownBits(Op.getOperand(0), DemandedElts, Depth + 1);
     Known = Known.zext(BitWidth);
----------------
dmgreen wrote:
> david-arm wrote:
> > I'm surprised we need so few bail-outs for all the opcodes for scalable vectors? For example, ISD::INTRINSIC_VOID, etc. call into target functions that may not yet deal with this properly. @paulwalker-arm is right that there was discussion about this during SVE sync calls with @efriedma  and Chris Tetreault and I remember distinctly that we were steered away (for what seemed like sensible reasons) from this approach. It's certainly worth having this discussion again now though. I think part of the problem is that you have to make sure every existing, and new, opcode takes sensible action for scalable vectors. Without a new interface to make it clear that DemandedElts now work with scalable vectors there is possibly a danger of introducing subtle bugs without realising it. At the very least it might be safer to explicitly call out all supported opcodes at the start of the function and bail out for any supported ones, before even getting to the switch statement?
> When I looked through all the opcodes I had not see anything that would cause problems, including into the AArch64 and RISCV computeKnownBitsForTargetNode methods. Did you have anything in particular that you think would be a problem?
> 
> Most interesting operations are lane-wise-identical, just passing DemandedElts through unchanged to other calls. That is the part that gives us a lot of benefit for SVE.
It's more that in general not teaching users that DemandedElts for this function now works for scalable vectors could lead to the silent introduction of bugs. For example,  does it only refer to the first known minimum number of elements, or does it refer to a repeating pattern of elements for each chunk? I think we should define explicitly what this means.

Also, here you're only changing the meaning of DemandedElts for one function, but there are lots of functions scattered throughout codegen and IR that have similar interfaces that take a DemandedElts. So there is now an inconsistency and is potentially confusing for users as to what it means. A quick grep of llvm/include/llvm showed around 65 declared functions that take a parameter of exactly the same name.

SelectionDAG::computeKnownBits is called by SelectionDAG::MaskedValueIsZero, SelectionDAG::MaskedVectorIsZero, TargetLowering::SimplifyMultipleUseDemandedBits, etc, which also take a DemandedElts parameter and pass it straight on to computeKnownBits. However, the comments on those interfaces haven't been updated to explain they now work for scalable vectors. Rather than go to the trouble of updating the comments on all those functions, I personally think it's easier to just create a new simple wrapper class to represent demanded elements in a similar way to what we did for ElementCount and TypeSize.


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

https://reviews.llvm.org/D128159