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

Thu Jun 30 07:25:52 PDT 2022

dmgreen added a comment.

Sorry for the delay - I became busy with some other things. I think I might put up the other patches I have so we can all take a look, as I don't see any huge reasons why not to go with this. We (for the moment) define DemandedElts for scalable vectors that all lanes must be demanded, which we represent as a DemandElts mask of VectorMinNumElements with all ones, and have asserts to make sure that is always true. It is simple, efficient, and gets us a lot of the benefits of KnownBits/SimplifyDemandedBits.

> 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.

It is defined to require that all lanes of the scalable vector are demanded.

> 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.

Like I said, I have patches for ComputeNumSignBits and SimplifyDemandedBits. This covers almost all of the uses of DemandedElts inside SDAG. There are other IR level functions, but they are separate.

> 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.

MaskedVectorIsZero and MaskedValueIsZero with DemandedElts are only actually called from the X86 backend. The others I have patches for, and haven't found any problems with them.

> 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.

A DemandedElts will not really be like ElementCount or TypeSize. It is not one thing (a count or type) optionally made scalable. It is a bitmask for each lane in a possibly effectively-infinite vector. It can't just be specified with a Bitmask that gets repeated. As far as I can tell, adding that might only be useful for BITCAST.

The ZERO/SIGN_EXTEND_VECTOR_INREG don't seem to be used for scalable vectors. Which really leaves EXTRACT_SUBVECTOR, EXTRACT_VECTOR_ELT and INSERT_SUBVECTOR/INSERT_VECTOR_ELEMENT, which are specific to the lane. They would probably need to be represented as "all lanes are 0 except these" or "all lanes are 1 except these", which are both useful for different places.

So I think DemandedElts would need to be a repeated pattern with some way to represent lanes that deviate from it, that needs to grow as far as needed for any constant lane indices. Plus all the logic needed to make the anding and oring and extracting work, but I've not looked into seeing how to implement them yet. I'm not against making something like that, but it needs to be added for a reason. Just handling BITCAST and the EXTRACT/INSERTS from scalable vectors doesn't feel like a huge reason to me compared to what else these functions provide.

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

https://reviews.llvm.org/D128159