[PATCH] D137140: [SDAG] Allow scalable vectors in ComputeKnownBits

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2982-2984
+    // TODO: Support zeroinitializer pattern
+    if (Op.getValueType().isScalableVector())
+      break;
----------------
I don't believe this is possible because `ISD::VECTOR_SHUFFLE` only supports fixed length vectors.  The zeroinitializer pattern you mention should be covered by `ISD::SPLAT_VECTOR`?

So I guess thus can be an assert like with the `ISD::BUILD_VECTOR` case.  Although both those ISD nodes are baked by a SDNode classes that should have asserts within their constructors and so we should have failed long before we get to this code.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3836-3838
+    // TODO: Probably okay to remove after audit; here to reduce change size
+    // in initial enablement patch for scalable vectors
+    if (Op.getValueType().isScalableVector())
----------------
I agree. It seems like if removing this causes issues then that indicates a target specific bug.  I did a quick parse of the `AArch64TargetLowering` version and nothing jumped out.  In the past we did share `AArch64ISD::DUP` across both vector types but I broke this a while back and forced all scalable vectors to use `ISD::SPLAT_VECTOR` instead.  That said, I think it's best to remove this as a separate patch so that it doesn't gate the rest of the code.


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

https://reviews.llvm.org/D137140