<div dir="ltr">Hi Philip,<div><br></div><div>I just reverted the patch. I'm happy to talk about this change on a proper patch.</div><div>I agree that the commit title is misleading, my apologies here.</div><div><br></div><div>FYI this patch and the previous ones are here to reduce the surface of the Align facility as much as possible in preparation for <a href="https://reviews.llvm.org/D128052">https://reviews.llvm.org/D128052</a>.</div><div><br></div><div>Thank you for reaching out to me,</div><div>Guillaume</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 22, 2022 at 4:43 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Er, I think this needs reverted, at minimum reapplied with a better <br>
comment, and probably reviewed.<br>
<br>
A couple problems here:<br>
<br>
1) This is not "dead code" in any definition I'm aware of. This is <br>
inlining a named helper function to make it dead, and then deleting it. <br>
As such, the submission comment is misleading and may cause interesting <br>
parties to miss it.<br>
<br>
2) I see clarity value in the original helper name. This is somewhat <br>
debatable, and I wouldn't treat this as blocking if someone else <br>
knowledgeable of the code was fine with the removal. Thus the review.<br>
<br>
Philip<br>
<br>
On 6/22/22 06:34, Guillaume Chatelet via llvm-commits wrote:<br>
> Author: Guillaume Chatelet<br>
> Date: 2022-06-22T13:33:58Z<br>
> New Revision: 8ba2cbff70f2c49a8926451c59cc260d67b706cf<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf.diff</a><br>
><br>
> LOG: [NFC] Remove dead code<br>
><br>
> Added:<br>
> <br>
><br>
> Modified:<br>
> llvm/include/llvm/Support/Alignment.h<br>
> llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp<br>
> llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
> llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> llvm/unittests/Support/AlignmentTest.cpp<br>
><br>
> Removed:<br>
> <br>
><br>
><br>
> ################################################################################<br>
> diff --git a/llvm/include/llvm/Support/Alignment.h b/llvm/include/llvm/Support/Alignment.h<br>
> index 0b1d52061f2c..11e574b600a3 100644<br>
> --- a/llvm/include/llvm/Support/Alignment.h<br>
> +++ b/llvm/include/llvm/Support/Alignment.h<br>
> @@ -208,10 +208,6 @@ inline uint64_t offsetToAlignedAddr(const void *Addr, Align Alignment) {<br>
> /// Returns the log2 of the alignment.<br>
> inline unsigned Log2(Align A) { return A.ShiftValue; }<br>
> <br>
> -/// Returns the alignment that satisfies both alignments.<br>
> -/// Same semantic as MinAlign.<br>
> -inline Align commonAlignment(Align A, Align B) { return std::min(A, B); }<br>
> -<br>
> /// Returns the alignment that satisfies both alignments.<br>
> /// Same semantic as MinAlign.<br>
> inline Align commonAlignment(Align A, uint64_t Offset) {<br>
><br>
> diff --git a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp<br>
> index bd100a297504..fb046d519ac8 100644<br>
> --- a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp<br>
> +++ b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp<br>
> @@ -7627,7 +7627,7 @@ LegalizerHelper::lowerMemcpy(MachineInstr &MI, Register Dst, Register Src,<br>
> <br>
> bool DstAlignCanChange = false;<br>
> MachineFrameInfo &MFI = MF.getFrameInfo();<br>
> - Align Alignment = commonAlignment(DstAlign, SrcAlign);<br>
> + Align Alignment = std::min(DstAlign, SrcAlign);<br>
> <br>
> MachineInstr *FIDef = getOpcodeDef(TargetOpcode::G_FRAME_INDEX, Dst, MRI);<br>
> if (FIDef && !MFI.isFixedObjectIndex(FIDef->getOperand(1).getIndex()))<br>
> @@ -7735,7 +7735,7 @@ LegalizerHelper::lowerMemmove(MachineInstr &MI, Register Dst, Register Src,<br>
> bool DstAlignCanChange = false;<br>
> MachineFrameInfo &MFI = MF.getFrameInfo();<br>
> bool OptSize = shouldLowerMemFuncForSize(MF);<br>
> - Align Alignment = commonAlignment(DstAlign, SrcAlign);<br>
> + Align Alignment = std::min(DstAlign, SrcAlign);<br>
> <br>
> MachineInstr *FIDef = getOpcodeDef(TargetOpcode::G_FRAME_INDEX, Dst, MRI);<br>
> if (FIDef && !MFI.isFixedObjectIndex(FIDef->getOperand(1).getIndex()))<br>
><br>
> diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> index fc031ce824d2..6dfc774d043f 100644<br>
> --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp<br>
> @@ -5863,7 +5863,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,<br>
> // @llvm.memcpy defines 0 and 1 to both mean no alignment.<br>
> Align DstAlign = MCI.getDestAlign().valueOrOne();<br>
> Align SrcAlign = MCI.getSourceAlign().valueOrOne();<br>
> - Align Alignment = commonAlignment(DstAlign, SrcAlign);<br>
> + Align Alignment = std::min(DstAlign, SrcAlign);<br>
> bool isVol = MCI.isVolatile();<br>
> bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());<br>
> // FIXME: Support passing<br>
> diff erent dest/src alignments to the memcpy DAG<br>
> @@ -5886,7 +5886,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,<br>
> // @llvm.memcpy.inline defines 0 and 1 to both mean no alignment.<br>
> Align DstAlign = MCI.getDestAlign().valueOrOne();<br>
> Align SrcAlign = MCI.getSourceAlign().valueOrOne();<br>
> - Align Alignment = commonAlignment(DstAlign, SrcAlign);<br>
> + Align Alignment = std::min(DstAlign, SrcAlign);<br>
> bool isVol = MCI.isVolatile();<br>
> bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());<br>
> // FIXME: Support passing<br>
> diff erent dest/src alignments to the memcpy DAG<br>
> @@ -5941,7 +5941,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,<br>
> // @llvm.memmove defines 0 and 1 to both mean no alignment.<br>
> Align DstAlign = MMI.getDestAlign().valueOrOne();<br>
> Align SrcAlign = MMI.getSourceAlign().valueOrOne();<br>
> - Align Alignment = commonAlignment(DstAlign, SrcAlign);<br>
> + Align Alignment = std::min(DstAlign, SrcAlign);<br>
> bool isVol = MMI.isVolatile();<br>
> bool isTC = I.isTailCall() && isInTailCallPosition(I, DAG.getTarget());<br>
> // FIXME: Support passing<br>
> diff erent dest/src alignments to the memmove DAG<br>
><br>
> diff --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
> index d87c8d77fffd..1f5bc69acecd 100644<br>
> --- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
> +++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp<br>
> @@ -773,7 +773,7 @@ bool MemCpyOptPass::processStore(StoreInst *SI, BasicBlock::iterator &BBI) {<br>
> LI, SI, SI->getPointerOperand()->stripPointerCasts(),<br>
> LI->getPointerOperand()->stripPointerCasts(),<br>
> DL.getTypeStoreSize(SI->getOperand(0)->getType()),<br>
> - commonAlignment(SI->getAlign(), LI->getAlign()), GetCall);<br>
> + std::min(SI->getAlign(), LI->getAlign()), GetCall);<br>
> if (changed) {<br>
> eraseInstruction(SI);<br>
> eraseInstruction(LI);<br>
><br>
> diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> index 631dbb35f450..2e5a150debc6 100644<br>
> --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp<br>
> @@ -3558,7 +3558,7 @@ static LoadsState canVectorizeLoads(ArrayRef<Value *> VL, const Value *VL0,<br>
> Align CommonAlignment = cast<LoadInst>(VL0)->getAlign();<br>
> for (Value *V : VL)<br>
> CommonAlignment =<br>
> - commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> + std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> auto *VecTy = FixedVectorType::get(ScalarTy, VL.size());<br>
> if (TTI.isLegalMaskedGather(VecTy, CommonAlignment) &&<br>
> !TTI.forceScalarizeMaskedGather(VecTy, CommonAlignment))<br>
> @@ -6438,7 +6438,7 @@ InstructionCost BoUpSLP::getEntryCost(const TreeEntry *E,<br>
> Align CommonAlignment = Alignment;<br>
> for (Value *V : VL)<br>
> CommonAlignment =<br>
> - commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> + std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> VecLdCost = TTI->getGatherScatterOpCost(<br>
> Instruction::Load, VecTy, cast<LoadInst>(VL0)->getPointerOperand(),<br>
> /*VariableMask=*/false, CommonAlignment, CostKind, VL0);<br>
> @@ -8150,7 +8150,7 @@ Value *BoUpSLP::vectorizeTree(TreeEntry *E) {<br>
> Align CommonAlignment = LI->getAlign();<br>
> for (Value *V : E->Scalars)<br>
> CommonAlignment =<br>
> - commonAlignment(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> + std::min(CommonAlignment, cast<LoadInst>(V)->getAlign());<br>
> NewLI = Builder.CreateMaskedGather(VecTy, VecPtr, CommonAlignment);<br>
> }<br>
> Value *V = propagateMetadata(NewLI, E->Scalars);<br>
><br>
> diff --git a/llvm/unittests/Support/AlignmentTest.cpp b/llvm/unittests/Support/AlignmentTest.cpp<br>
> index 0514db28a243..0baec54b3380 100644<br>
> --- a/llvm/unittests/Support/AlignmentTest.cpp<br>
> +++ b/llvm/unittests/Support/AlignmentTest.cpp<br>
> @@ -115,21 +115,6 @@ TEST(AlignmentTest, Log2) {<br>
> }<br>
> }<br>
> <br>
> -TEST(AlignmentTest, MinAlign) {<br>
> - struct {<br>
> - uint64_t A;<br>
> - uint64_t B;<br>
> - uint64_t MinAlign;<br>
> - } kTests[] = {<br>
> - {1, 2, 1},<br>
> - {8, 4, 4},<br>
> - };<br>
> - for (const auto &T : kTests) {<br>
> - EXPECT_EQ(MinAlign(T.A, T.B), T.MinAlign);<br>
> - EXPECT_EQ(commonAlignment(Align(T.A), Align(T.B)), T.MinAlign);<br>
> - }<br>
> -}<br>
> -<br>
> TEST(AlignmentTest, Encode_Decode) {<br>
> for (uint64_t Value : getValidAlignments()) {<br>
> {<br>
><br>
><br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>