[SVE][CodeGen][NFC] Replace TypeSize comparison operators with their scalar equivalents

[david-arm]

Summary

In certain places in llvm/lib/CodeGen we were relying upon the TypeSize
comparison operators when in fact the code was only ever expecting
either scalar values or fixed width vectors. I've changed some of these
places to use the equivalent scalar operator.

[efriedma]

Please separate all the "non-functional" (just adding .getFixedSize()) changes into a separate patch from any changes that potentially have functional effects.

[david-arm]

Hi @efriedma, sure I can do that. To be honest (and this is probably my lack of understanding of what is meant by a functional change) I thought they were almost all non-functional. That's why I originally put them in this patch together. Just for my understanding, which bits do you consider non-functional? I can definitely see that one change in TargetLowering is non-functional, but in other cases such as mergeTruncStores where I call getScalarSizeInBits() instead of getSizeInBits() to me, naively, this seemed non-functional because we're dealing with scalar values - the function bails out if the type is not a simple integer. Similarly in ReduceLoadWidth we bail out immediately if the type is a vector, so from that point onwards the type is guaranteed to be scalar.

[sdesmalen]

From a fixed-width vector perspective, all the getFixedSize() changes are purely NFC. Changes like getSizeInBits() -> getScalarSizeInBits() are _possibly_ functional if your assumption that the type must be a scalar value, ends up being false. I think the exception here are the cases where it can be trivially seen that the function bails out early when the type is not a scalar.

From a scalable-vector perspective, the getFixedSize() changes are not necessarily non-functional, because if a scalable vector goes down these code-paths things will now break with an assert where it would previously 'just work'. In your patch you're making the assumption that scalable vectors shouldn't go down these code-paths, so they are likely to be non-functional.

I think Eli's point is that you could move out all the getFixedSize changes into a separate patch, as those are known to be NFC for fixed-width vectors and can't break any of the buildbots or impact any other users of LLVM that don't compile for SVE.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

7862

Was this change supposed to be part of this patch?

[kmclaughlin]

LGTM

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

case Instruction::PtrToInt: {
 
     const MCExpr *OpExpr = lowerConstant(Op);
 
     // We can emit the pointer value into this slot if the slot is an
     // integer slot equal to the size of the pointer.
     //
     // If the pointer is larger than the resultant integer, then
     // as with Trunc just depend on the assembler to truncate it.
-    if (DL.getTypeAllocSize(Ty) <= DL.getTypeAllocSize(Op->getType()))
+    if (DL.getTypeAllocSize(Ty).getFixedSize() <=
+        DL.getTypeAllocSize(Op->getType()).getFixedSize())
       return OpExpr;
 
     // Otherwise the pointer is smaller than the resultant integer, mask off
     // the high bits so we are sure to get a proper truncation if the input is
     // a constant expr.
     unsigned InBits = DL.getTypeAllocSizeInBits(Op->getType());
     const MCExpr *MaskExpr = MCConstantExpr::create(~0ULL >> (64-InBits), Ctx);
     return MCBinaryExpr::createAnd(OpExpr, MaskExpr, Ctx);

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp

for (unsigned Part = 0; Part < NumArgRegs; ++Part) {
         Register StackAddr = Handler.getStackAddress(MemSize, Offset, MPO);
         Handler.assignValueToAddress(Args[i], StackAddr,
                                      MemSize, MPO, VA);
         continue;
       }
 
       assert(VA.isRegLoc() && "custom loc should have been handled already");
 
-      if (OrigVT.getSizeInBits() >= VAVT.getSizeInBits() ||
+      // GlobalISel does not currently work for scalable vectors.
+      if (OrigVT.getFixedSizeInBits() >= VAVT.getFixedSizeInBits() ||
           !Handler.isIncomingArgumentHandler()) {
         // This is an argument that might have been split. There should be
         // Regs.size() ArgLocs per argument.
 
         // Insert the argument copies. If VAVT < OrigVT, we'll insert the merge
         // to the original register after handling all of the parts.
         Handler.assignValueToReg(Args[i].Regs[Part], VA.getLocReg(), VA);
         continue;
for (unsigned Part = 0; Part < NumArgRegs; ++Part) {
       } else {
         MIRBuilder.buildTrunc(ArgReg, {NewReg}).getReg(0);
       }
     }
 
     // Now that all pieces have been handled, re-pack any arguments into any
     // wider, original registers.
     if (Handler.isIncomingArgumentHandler()) {
-      if (VAVT.getSizeInBits() < OrigVT.getSizeInBits()) {
+      if (VAVT.getFixedSizeInBits() < OrigVT.getFixedSizeInBits()) {
         assert(NumArgRegs >= 2);
 
         // Merge the split registers into the expected larger result vreg
         // of the original call.
         MIRBuilder.buildMerge(Args[i].OrigRegs[0], Args[i].Regs);
       }
     }
 

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::ReduceLoadWidth(SDNode *N) {
   LoadSDNode *LN0 = cast<LoadSDNode>(N0);
   // Reducing the width of a volatile load is illegal.  For atomics, we may be
   // able to reduce the width provided we never widen again. (see D66309)
   if (!LN0->isSimple() ||
       !isLegalNarrowLdSt(LN0, ExtType, ExtVT, ShAmt))
     return SDValue();
 
   auto AdjustBigEndianShift = [&](unsigned ShAmt) {
-    unsigned LVTStoreBits = LN0->getMemoryVT().getStoreSizeInBits();
-    unsigned EVTStoreBits = ExtVT.getStoreSizeInBits();
+    unsigned LVTStoreBits =
+        LN0->getMemoryVT().getStoreSizeInBits().getFixedSize();
+    unsigned EVTStoreBits = ExtVT.getStoreSizeInBits().getFixedSize();
     return LVTStoreBits - EVTStoreBits - ShAmt;
   };
 
   // For big endian targets, we need to adjust the offset to the pointer to
   // load the correct bytes.
   if (DAG.getDataLayout().isBigEndian())
     ShAmt = AdjustBigEndianShift(ShAmt);
 

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

void DAGTypeLegalizer::SetSoftPromotedHalf(SDValue Op, SDValue Result) {
   assert((OpIdEntry == 0) && "Node is already promoted!");
   OpIdEntry = getTableId(Result);
 }
 
 void DAGTypeLegalizer::SetScalarizedVector(SDValue Op, SDValue Result) {
   // Note that in some cases vector operation operands may be greater than
   // the vector element type. For example BUILD_VECTOR of type <1 x i1> with
   // a constant i8 operand.
-  assert(Result.getValueSizeInBits() >= Op.getScalarValueSizeInBits() &&
+
+  // We don't currently support the scalarization of scalable vector types.
+  assert(Result.getValueSizeInBits().getFixedSize() >=
+             Op.getScalarValueSizeInBits() &&
          "Invalid type for scalarized vector");
   AnalyzeNewValue(Result);
 
   auto &OpIdEntry = ScalarizedVectors[getTableId(Op)];
   assert((OpIdEntry == 0) && "Node is already scalarized!");
   OpIdEntry = getTableId(Result);
 }
 

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::stackrestore:
     DAG.setRoot(DAG.getNode(ISD::STACKRESTORE, sdl, MVT::Other, getRoot(), Res));
     return;
   case Intrinsic::get_dynamic_area_offset: {
     SDValue Op = getRoot();
     EVT PtrTy = TLI.getFrameIndexTy(DAG.getDataLayout());
     EVT ResTy = TLI.getValueType(DAG.getDataLayout(), I.getType());
     // Result type for @llvm.get.dynamic.area.offset should match PtrTy for
     // target.
-    if (PtrTy.getSizeInBits() < ResTy.getSizeInBits())
+    if (PtrTy.getFixedSizeInBits() < ResTy.getFixedSizeInBits())
       report_fatal_error("Wrong result type for @llvm.get.dynamic.area.offset"
                          " intrinsic!");
     Res = DAG.getNode(ISD::GET_DYNAMIC_AREA_OFFSET, sdl, DAG.getVTList(ResTy),
                       Op);
     DAG.setRoot(Op);
     setValue(&I, Res);
     return;
   }

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (C.isPowerOf2()) {
       return true;
     }
   }
 
   EVT WideVT = EVT::getIntegerVT(*DAG.getContext(), VT.getScalarSizeInBits() * 2);
   if (VT.isVector())
     WideVT = EVT::getVectorVT(*DAG.getContext(), WideVT,
                               VT.getVectorNumElements());
 
   SDValue BottomHalf;
   SDValue TopHalf;
   static const unsigned Ops[2][3] =
       { { ISD::MULHU, ISD::UMUL_LOHI, ISD::ZERO_EXTEND },
         { ISD::MULHS, ISD::SMUL_LOHI, ISD::SIGN_EXTEND }};
   if (isOperationLegalOrCustom(Ops[isSigned][0], VT)) {
     BottomHalf = DAG.getNode(ISD::MUL, dl, VT, LHS, RHS);
     TopHalf = DAG.getNode(Ops[isSigned][0], dl, VT, LHS, RHS);
else if (WideVT == MVT::i128)
       LC = RTLIB::MUL_I128;
     assert(LC != RTLIB::UNKNOWN_LIBCALL && "Cannot expand this operation!");
 
     SDValue HiLHS;
     SDValue HiRHS;
     if (isSigned) {
       // The high part is obtained by SRA'ing all but one of the bits of low
       // part.
-      unsigned LoSize = VT.getSizeInBits();
+      unsigned LoSize = VT.getFixedSizeInBits();
       HiLHS =
           DAG.getNode(ISD::SRA, dl, VT, LHS,
                       DAG.getConstant(LoSize - 1, dl,
                                       getPointerTy(DAG.getDataLayout())));
       HiRHS =
           DAG.getNode(ISD::SRA, dl, VT, RHS,
                       DAG.getConstant(LoSize - 1, dl,
                                       getPointerTy(DAG.getDataLayout())));

llvm/lib/CodeGen/TargetLoweringBase.cpp

case TypePromoteInteger: {
                                    MVT::LAST_INTEGER_FIXEDLEN_VECTOR_VALUETYPE;
       // Try to promote the elements of integer vectors. If no legal
       // promotion was found, fall through to the widen-vector method.
       for (unsigned nVT = i + 1;
            (MVT::SimpleValueType)nVT <= EndVT; ++nVT) {
         MVT SVT = (MVT::SimpleValueType) nVT;
         // Promote vectors of integers to vectors with the same number
         // of elements, with a wider element type.
-        if (SVT.getScalarSizeInBits() > EltVT.getSizeInBits() &&
+        if (SVT.getScalarSizeInBits() > EltVT.getFixedSizeInBits() &&
             SVT.getVectorElementCount() == EC && isTypeLegal(SVT)) {
           TransformToType[i] = SVT;
           RegisterTypeForVT[i] = SVT;
           NumRegistersForVT[i] = 1;
           ValueTypeActions.setTypeAction(VT, TypePromoteInteger);
           IsLegalWiderType = true;
           break;
         }