[Alignment] Replace commonAlignment with std::min

[gchatelet]

Summary

commonAlignment is a shortcut to pick the smallest of two Align
objects. As-is it doesn't bring much value compared to std::min.

[gchatelet]

I've added the authors of the code as reviewers so we can discuss if std::min makes sense or not

[gchatelet]

@aemerson @bkramer is std::min(Align, Align) clear enough?

[bkramer]

In D128345#3607911, @gchatelet wrote:

@aemerson @bkramer is std::min(Align, Align) clear enough?

I think so. But I find it rather confusing to have the other overload of commonAlign still around. Are you planning to remove that?

[gchatelet]

In D128345#3607978, @bkramer wrote:

In D128345#3607911, @gchatelet wrote:

@aemerson @bkramer is std::min(Align, Align) clear enough?

I think so. But I find it rather confusing to have the other overload of commonAlign still around. Are you planning to remove that?

I didn't plan to remove it. They convey slightly different semantics:

• commonAlignment(Align, Align) is the smallest of the alignments (they are both valid alignments by construction)
• commonAlignment(Align, uint64_t) or MinAlign(uint64_t, uint64_t)as defined in MathExtras.h return the alignment that satisfies both Lhs and Rhs

The discrepancy came from the introduction of the type, before Align everything was working on integer types and used MinAlign.
Now we have a proper type and in this case picking the common alignment is the same as taking the minimum.
When we start with integer types we have to do more work. Note that these functions are also less safe as integer promotion can kick in, they will happily take an unsigned type as input.

What would you suggest? We can keep both functions if you think it makes sense, or find a better name for the remaining function?

[bkramer]

I think given the semantic difference removing just the one overload makes sense.

[gchatelet]

Thx for the review!

llvm/include/llvm/Support/Alignment.h

inline uint64_t offsetToAlignedAddr(const void *Addr, Align Alignment) {
   return offsetToAlignment(reinterpret_cast<uintptr_t>(Addr), Alignment);
 }
 
 /// Returns the log2 of the alignment.
 inline unsigned Log2(Align A) { return A.ShiftValue; }
 
 /// Returns the alignment that satisfies both alignments.
 /// Same semantic as MinAlign.
-inline Align commonAlignment(Align A, Align B) { return std::min(A, B); }
-
-/// Returns the alignment that satisfies both alignments.
-/// Same semantic as MinAlign.
 inline Align commonAlignment(Align A, uint64_t Offset) {
   return Align(MinAlign(A.value(), Offset));
 }
 
 /// Returns a representation of the alignment that encodes undefined as 0.
 inline unsigned encode(MaybeAlign A) { return A ? A->ShiftValue + 1 : 0; }
 
 /// Dual operation of the encode function above.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

LegalizerHelper::lowerMemcpy(MachineInstr &MI, Register Dst, Register Src,
   const auto &TLI = *MF.getSubtarget().getTargetLowering();
   auto &DL = MF.getDataLayout();
   LLVMContext &C = MF.getFunction().getContext();
 
   assert(KnownLen != 0 && "Have a zero length memcpy length!");
 
   bool DstAlignCanChange = false;
   MachineFrameInfo &MFI = MF.getFrameInfo();
-  Align Alignment = commonAlignment(DstAlign, SrcAlign);
+  Align Alignment = std::min(DstAlign, SrcAlign);
 
   MachineInstr *FIDef = getOpcodeDef(TargetOpcode::G_FRAME_INDEX, Dst, MRI);
   if (FIDef && !MFI.isFixedObjectIndex(FIDef->getOperand(1).getIndex()))
     DstAlignCanChange = true;
 
   // FIXME: infer better src pointer alignment like SelectionDAG does here.
   // FIXME: also use the equivalent of isMemSrcFromConstant and alwaysinlining
   // if the memcpy is in a tail call position.
LegalizerHelper::lowerMemmove(MachineInstr &MI, Register Dst, Register Src,
   auto &DL = MF.getDataLayout();
   LLVMContext &C = MF.getFunction().getContext();
 
   assert(KnownLen != 0 && "Have a zero length memmove length!");
 
   bool DstAlignCanChange = false;
   MachineFrameInfo &MFI = MF.getFrameInfo();
   bool OptSize = shouldLowerMemFuncForSize(MF);
-  Align Alignment = commonAlignment(DstAlign, SrcAlign);
+  Align Alignment = std::min(DstAlign, SrcAlign);
 
   MachineInstr *FIDef = getOpcodeDef(TargetOpcode::G_FRAME_INDEX, Dst, MRI);
   if (FIDef && !MFI.isFixedObjectIndex(FIDef->getOperand(1).getIndex()))
     DstAlignCanChange = true;
 
   unsigned Limit = TLI.getMaxStoresPerMemmove(OptSize);
   std::vector<LLT> MemOps;
 

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
   case Intrinsic::memcpy: {
     const auto &MCI = cast<MemCpyInst>(I);
     SDValue Op1 = getValue(I.getArgOperand(0));
     SDValue Op2 = getValue(I.getArgOperand(1));
     SDValue Op3 = getValue(I.getArgOperand(2));
     // @llvm.memcpy defines 0 and 1 to both mean no alignment.
     Align DstAlign = MCI.getDestAlign().valueOrOne();
     Align SrcAlign = MCI.getSourceAlign().valueOrOne();
-    Align Alignment = commonAlignment(DstAlign, SrcAlign);
+    Align Alignment = std::min(DstAlign, SrcAlign);
     bool isVol = MCI.isVolatile();
     bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());
     // FIXME: Support passing different dest/src alignments to the memcpy DAG
     // node.
     SDValue Root = isVol ? getRoot() : getMemoryRoot();
     SDValue MC = DAG.getMemcpy(Root, sdl, Op1, Op2, Op3, Alignment, isVol,
                                /* AlwaysInline */ false, isTC,
                                MachinePointerInfo(I.getArgOperand(0)),
                                MachinePointerInfo(I.getArgOperand(1)),
                                I.getAAMetadata());
     updateDAGForMaybeTailCall(MC);
     return;
   }
   case Intrinsic::memcpy_inline: {
     const auto &MCI = cast<MemCpyInlineInst>(I);
     SDValue Dst = getValue(I.getArgOperand(0));
     SDValue Src = getValue(I.getArgOperand(1));
     SDValue Size = getValue(I.getArgOperand(2));
     assert(isa<ConstantSDNode>(Size) && "memcpy_inline needs constant size");
     // @llvm.memcpy.inline defines 0 and 1 to both mean no alignment.
     Align DstAlign = MCI.getDestAlign().valueOrOne();
     Align SrcAlign = MCI.getSourceAlign().valueOrOne();
-    Align Alignment = commonAlignment(DstAlign, SrcAlign);
+    Align Alignment = std::min(DstAlign, SrcAlign);
     bool isVol = MCI.isVolatile();
     bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());
     // FIXME: Support passing different dest/src alignments to the memcpy DAG
     // node.
     SDValue MC = DAG.getMemcpy(getRoot(), sdl, Dst, Src, Size, Alignment, isVol,
                                /* AlwaysInline */ true, isTC,
                                MachinePointerInfo(I.getArgOperand(0)),
                                MachinePointerInfo(I.getArgOperand(1)),
void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
   case Intrinsic::memmove: {
     const auto &MMI = cast<MemMoveInst>(I);
     SDValue Op1 = getValue(I.getArgOperand(0));
     SDValue Op2 = getValue(I.getArgOperand(1));
     SDValue Op3 = getValue(I.getArgOperand(2));
     // @llvm.memmove defines 0 and 1 to both mean no alignment.
     Align DstAlign = MMI.getDestAlign().valueOrOne();
     Align SrcAlign = MMI.getSourceAlign().valueOrOne();
-    Align Alignment = commonAlignment(DstAlign, SrcAlign);
+    Align Alignment = std::min(DstAlign, SrcAlign);
     bool isVol = MMI.isVolatile();
     bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());
     // FIXME: Support passing different dest/src alignments to the memmove DAG
     // node.
     SDValue Root = isVol ? getRoot() : getMemoryRoot();
     SDValue MM = DAG.getMemmove(Root, sdl, Op1, Op2, Op3, Alignment, isVol,
                                 isTC, MachinePointerInfo(I.getArgOperand(0)),
                                 MachinePointerInfo(I.getArgOperand(1)),

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

if (LI->isSimple() && LI->hasOneUse() &&
           return dyn_cast_or_null<CallInst>(LoadClobber->getMemoryInst());
         return nullptr;
       };
 
       bool changed = performCallSlotOptzn(
           LI, SI, SI->getPointerOperand()->stripPointerCasts(),
           LI->getPointerOperand()->stripPointerCasts(),
           DL.getTypeStoreSize(SI->getOperand(0)->getType()),
-          commonAlignment(SI->getAlign(), LI->getAlign()), GetCall);
+          std::min(SI->getAlign(), LI->getAlign()), GetCall);
       if (changed) {
         eraseInstruction(SI);
         eraseInstruction(LI);
         ++NumMemCpyInstr;
         return true;
       }
     }
   }

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

if (IsSorted || all_of(PointerOps, [&PointerOps](Value *P) {
     if (ProfitableGatherPointers || all_of(PointerOps, [IsSorted](Value *P) {
           auto *GEP = dyn_cast<GetElementPtrInst>(P);
           return (IsSorted && !GEP && doesNotNeedToBeScheduled(P)) ||
                  (GEP && GEP->getNumOperands() == 2);
         })) {
       Align CommonAlignment = cast<LoadInst>(VL0)->getAlign();
       for (Value *V : VL)
         CommonAlignment =
-            commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());
+            std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());
       auto *VecTy = FixedVectorType::get(ScalarTy, VL.size());
       if (TTI.isLegalMaskedGather(VecTy, CommonAlignment) &&
           !TTI.forceScalarizeMaskedGather(VecTy, CommonAlignment))
         return LoadsState::ScatterVectorize;
     }
   }
 
   return LoadsState::Gather;
case Instruction::Load: {
       if (E->State == TreeEntry::Vectorize) {
         VecLdCost = TTI->getMemoryOpCost(Instruction::Load, VecTy, Alignment, 0,
                                          CostKind, VL0);
       } else {
         assert(E->State == TreeEntry::ScatterVectorize && "Unknown EntryState");
         Align CommonAlignment = Alignment;
         for (Value *V : VL)
           CommonAlignment =
-              commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());
+              std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());
         VecLdCost = TTI->getGatherScatterOpCost(
             Instruction::Load, VecTy, cast<LoadInst>(VL0)->getPointerOperand(),
             /*VariableMask=*/false, CommonAlignment, CostKind, VL0);
       }
       LLVM_DEBUG(dumpTreeCosts(E, CommonCost, VecLdCost, ScalarLdCost));
       return CommonCost + VecLdCost - ScalarLdCost;
     }
     case Instruction::Store: {
case Instruction::Load: {
         }
       } else {
         assert(E->State == TreeEntry::ScatterVectorize && "Unhandled state");
         Value *VecPtr = vectorizeTree(E->getOperand(0));
         // Use the minimum alignment of the gathered loads.
         Align CommonAlignment = LI->getAlign();
         for (Value *V : E->Scalars)
           CommonAlignment =
-              commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());
+              std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());
         NewLI = Builder.CreateMaskedGather(VecTy, VecPtr, CommonAlignment);
       }
       Value *V = propagateMetadata(NewLI, E->Scalars);
 
       ShuffleBuilder.addInversedMask(E->ReorderIndices);
       ShuffleBuilder.addMask(E->ReuseShuffleIndices);
       V = ShuffleBuilder.finalize(V);
       E->VectorizedValue = V;

llvm/unittests/Support/AlignmentTest.cpp

 }
 
 TEST(AlignmentTest, Log2) {
   for (uint64_t Value : getValidAlignments()) {
     EXPECT_EQ(Log2(Align(Value)), Log2_64(Value));
   }
 }
 
-TEST(AlignmentTest, MinAlign) {
-  struct {
-    uint64_t A;
-    uint64_t B;
-    uint64_t MinAlign;
-  } kTests[] = {
-      {1, 2, 1},
-      {8, 4, 4},
-  };
-  for (const auto &T : kTests) {
-    EXPECT_EQ(MinAlign(T.A, T.B), T.MinAlign);
-    EXPECT_EQ(commonAlignment(Align(T.A), Align(T.B)), T.MinAlign);
-  }
-}
-
 TEST(AlignmentTest, Encode_Decode) {
   for (uint64_t Value : getValidAlignments()) {
     {
       Align Actual(Value);
       Align Expected = *decodeMaybeAlign(encode(Actual));
       EXPECT_EQ(Expected, Actual);
     }
     {