[PATCH] D88654: [SVE][CodeGen] Replace uses of TypeSize comparison operators with calls to isKnownXY

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I guess most of my comments relate to minimising direct uses of TypeSize.



================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:950
+        // same scalable property.
+        TypeSize::isKnownLT(Src->getPrimitiveSizeInBits(), LT.second.getSizeInBits())) {
       // This is a vector load that legalizes to a larger type than the vector
----------------
Just a thought but if you get the EVT for Src then you could use bitsLT and then the comment is not needed as it'll assert as much.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4967-4973
+  TypeSize LdStMemSize = LDST->getMemoryVT().getSizeInBits();
+  TypeSize NewMemSize = MemVT.getSizeInBits();
+  if (TypeSize::isKnownLT(LdStMemSize, NewMemSize) ||
+      // Fow now let's be conservative and bail out when changing the
+      // scalable property, since we can't be sure that we're actually
+      // narrowing here.
+      LdStMemSize.isScalable() != NewMemSize.isScalable())
----------------
Perhaps worth breaking this out into separate blocks. e.g.
```
LdStVT = LDST->getMemoryVT();

//Don't convert between fixed length and scalable types.
if (LdStVT.isScalable() != MemVT.isScalable())
  return false;

if (LdStVT.bitsLT(MemVT))
......
```
What do you think?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11493
       if (LN0->isSimple() &&
-          LN0->getMemoryVT().getStoreSizeInBits() < VT.getSizeInBits()) {
+          TypeSize::isKnownLT(LN0->getMemoryVT().getStoreSizeInBits(), VT.getSizeInBits())) {
         SDValue NewLoad = DAG.getExtLoad(LN0->getExtensionType(), SDLoc(LN0),
----------------
I got bored scrolling up but I believe this function is visitTRUNCATE and thus `N0` has to have the same number of lanes as VT and thus `bitsLT` can be used?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1825
+  if (SrcVT.getVectorElementCount().isKnownEven() &&
+      TypeSize::isKnownLT(SrcVT.getSizeInBits() * 2, DestVT.getSizeInBits())) {
     LLVMContext &Ctx = *DAG.getContext();
----------------
Extends have to have the same number of lanes so I think this case can be converted to compare the size of the vector elements and thus use `SrcVT.getScalarSizeInBits() * 2 < DestVT.getScalarSizeInBits()`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:674-675
                "lossy conversion of vector to scalar type");
         EVT IntermediateType =
             EVT::getIntegerVT(*DAG.getContext(), ValueVT.getSizeInBits());
         Val = DAG.getBitcast(IntermediateType, Val);
----------------
My reading of this is that we're trying to create a scalar that's as big as the vector ValueVT.  We don't support scalable scalar types and thus I'm wondering if the above assert can just use getFixedSizeInBits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88654