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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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);
----------------
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.


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

https://reviews.llvm.org/D128159