[llvm] 8ba2cbf - [NFC] Remove dead code
Guillaume Chatelet via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 22 08:01:07 PDT 2022
Hi Philip,
I just reverted the patch. I'm happy to talk about this change on a proper
patch.
I agree that the commit title is misleading, my apologies here.
FYI this patch and the previous ones are here to reduce the surface of the
Align facility as much as possible in preparation for
https://reviews.llvm.org/D128052.
Thank you for reaching out to me,
Guillaume
On Wed, Jun 22, 2022 at 4:43 PM Philip Reames <listmail at philipreames.com>
wrote:
> Er, I think this needs reverted, at minimum reapplied with a better
> comment, and probably reviewed.
>
> A couple problems here:
>
> 1) This is not "dead code" in any definition I'm aware of. This is
> inlining a named helper function to make it dead, and then deleting it.
> As such, the submission comment is misleading and may cause interesting
> parties to miss it.
>
> 2) I see clarity value in the original helper name. This is somewhat
> debatable, and I wouldn't treat this as blocking if someone else
> knowledgeable of the code was fine with the removal. Thus the review.
>
> Philip
>
> On 6/22/22 06:34, Guillaume Chatelet via llvm-commits wrote:
> > Author: Guillaume Chatelet
> > Date: 2022-06-22T13:33:58Z
> > New Revision: 8ba2cbff70f2c49a8926451c59cc260d67b706cf
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf
> > DIFF:
> https://github.com/llvm/llvm-project/commit/8ba2cbff70f2c49a8926451c59cc260d67b706cf.diff
> >
> > LOG: [NFC] Remove dead code
> >
> > Added:
> >
> >
> > Modified:
> > llvm/include/llvm/Support/Alignment.h
> > llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
> > llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
> > llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> > llvm/unittests/Support/AlignmentTest.cpp
> >
> > Removed:
> >
> >
> >
> >
> ################################################################################
> > diff --git a/llvm/include/llvm/Support/Alignment.h
> b/llvm/include/llvm/Support/Alignment.h
> > index 0b1d52061f2c..11e574b600a3 100644
> > --- a/llvm/include/llvm/Support/Alignment.h
> > +++ b/llvm/include/llvm/Support/Alignment.h
> > @@ -208,10 +208,6 @@ inline uint64_t offsetToAlignedAddr(const void
> *Addr, Align 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) {
> >
> > diff --git a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
> b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
> > index bd100a297504..fb046d519ac8 100644
> > --- a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
> > +++ b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
> > @@ -7627,7 +7627,7 @@ LegalizerHelper::lowerMemcpy(MachineInstr &MI,
> Register Dst, Register Src,
> >
> > 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()))
> > @@ -7735,7 +7735,7 @@ LegalizerHelper::lowerMemmove(MachineInstr &MI,
> Register Dst, Register Src,
> > 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()))
> >
> > diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > index fc031ce824d2..6dfc774d043f 100644
> > --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > @@ -5863,7 +5863,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const
> CallInst &I,
> > // @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
> > diff erent dest/src alignments to the memcpy DAG
> > @@ -5886,7 +5886,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const
> CallInst &I,
> > // @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
> > diff erent dest/src alignments to the memcpy DAG
> > @@ -5941,7 +5941,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const
> CallInst &I,
> > // @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
> > diff erent dest/src alignments to the memmove DAG
> >
> > diff --git a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
> b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
> > index d87c8d77fffd..1f5bc69acecd 100644
> > --- a/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
> > +++ b/llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
> > @@ -773,7 +773,7 @@ bool MemCpyOptPass::processStore(StoreInst *SI,
> BasicBlock::iterator &BBI) {
> > 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);
> >
> > diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> > index 631dbb35f450..2e5a150debc6 100644
> > --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> > +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> > @@ -3558,7 +3558,7 @@ static LoadsState canVectorizeLoads(ArrayRef<Value
> *> VL, const Value *VL0,
> > 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))
> > @@ -6438,7 +6438,7 @@ InstructionCost BoUpSLP::getEntryCost(const
> TreeEntry *E,
> > 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);
> > @@ -8150,7 +8150,7 @@ Value *BoUpSLP::vectorizeTree(TreeEntry *E) {
> > 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);
> >
> > diff --git a/llvm/unittests/Support/AlignmentTest.cpp
> b/llvm/unittests/Support/AlignmentTest.cpp
> > index 0514db28a243..0baec54b3380 100644
> > --- a/llvm/unittests/Support/AlignmentTest.cpp
> > +++ b/llvm/unittests/Support/AlignmentTest.cpp
> > @@ -115,21 +115,6 @@ TEST(AlignmentTest, Log2) {
> > }
> > }
> >
> > -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()) {
> > {
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220622/65f22fc0/attachment.html>
More information about the llvm-commits
mailing list