[PATCH] D80919: [CodeGen] Fix warnings in getCopyFromPartsVector

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 3 inline comments as done.
david-arm added a comment.

I'll give this patch some more thought for now and see if I can just simply remove the assert in getCopy...



================
Comment at: llvm/include/llvm/Support/TypeSize.h:56
+  // answer at compile time.
+  bool isGT(const ElementCount &RHS, bool &Known) {
+    bool Result;
----------------
rovka wrote:
> Is it ever safe to ignore the value of Known? If not, I'd suggest making this method private so people are forced to use knownGT. Alternatively, you could define a 'compare' function and have it return an enum value saying Unknown, GT, LE etc.
I think it's more the other way around really. In many cases we will probably be trying to apply an optimisation based on whether the result is true or false. If the answer cannot be determined (Known=false), then we shouldn't be applying that transformation or going down that particular code path. I'll think more about this though and perhaps I can refactor the existing code in getCopyFromParts such that we don't even need to ask this question.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:64
+    } else
+      Known = Result = false;
+    return Result;
----------------
rovka wrote:
> To be honest I find this whole sequence a bit difficult to follow. Shouldn't Known only be true if LHS and RHS are either both scalable or both fixed? If so, then you can just say Known = (Scalable == RHS.Scalable). Similar to how it's handled for TypeSize. This looks like another argument in favor of the alternative design with an enum, which just separates these concerns and makes you decide up front whether or not it's possible to make the comparison.
So the Min variable represents the absolute minimum number of elements. Therefore, when LHS is a fixed width vector the element count is guaranteed to always be <=the element count of RHS, regardless of whether it's scalable or not. Hence the odd looking code! I could always return "Known = false" when Scalable != RHS.Scalable, but I just thought we can cover more cases than that.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:443
     if (PartEVT.getVectorElementType() == ValueVT.getVectorElementType()) {
-      assert(PartEVT.getVectorNumElements() > ValueVT.getVectorNumElements() &&
+      assert(PartEC.knownGT(ValueEC) &&
              "Cannot narrow, it would be a lossy transformation");
----------------
rovka wrote:
> If we know that the vector element types are the same, can't we just compare the type sizes here and piggy-back on the comparison operators we already have instead of adding new ones?
So we can't really piggyback on the TypeSize class here as they are quite different classes, although I could certainly duplicate the operators. The operators in TypeSize simply assert that both LHS and RHS are either fixed or scalable types. I guess this will often be the case, but is not guaranteed. To be honest, TypeSize and ElementCount will need to be refactored at some point to make use of some common base class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80919