[llvm] r275042 - Revert r275027 - Let FuncAttrs infer the 'returned' argument attribute
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Sat Jul 16 00:16:00 PDT 2016
----- Original Message -----
> From: "James Molloy" <James.Molloy at arm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Tim Northover" <t.p.northover at gmail.com>, "renato golin" <renato.golin at linaro.org>, llvm-commits at lists.llvm.org
> Sent: Friday, July 15, 2016 12:21:06 PM
> Subject: Re: [llvm] r275042 - Revert r275027 - Let FuncAttrs infer the 'returned' argument attribute
>
> Hi Hal,
>
> That seems reasonable to me.
r275677. I'll reapply this inference patch and see how it goes.
Thanks again,
Hal
>
> James
>
> > On 15 Jul 2016, at 18:16, Hal Finkel <hfinkel at anl.gov> wrote:
> >
> > Hi James, et al.,
> >
> > Can we put this under a flag for the time being, as it seems that
> > it can trigger miscompilation in some subtle cases, until we can
> > track down the problem?
> >
> > Thanks again,
> > Hal
> >
> > ----- Original Message -----
> >> From: "James Molloy" <James.Molloy at arm.com>
> >> To: "Hal Finkel" <hfinkel at anl.gov>
> >> Cc: "Tim Northover" <t.p.northover at gmail.com>, "renato golin"
> >> <renato.golin at linaro.org>, llvm-commits at lists.llvm.org
> >> Sent: Monday, July 11, 2016 9:05:30 AM
> >> Subject: Re: [llvm] r275042 - Revert r275027 - Let FuncAttrs infer
> >> the 'returned' argument attribute
> >>
> >> Hi,
> >>
> >>
> >> After investigating, I still haven’t found the issue here. The
> >> code
> >> looks sensible, and there are only two places where the returned
> >> attribute affects codegen: setting the preserved mask and marking
> >> the zero’th parameter as used:
> >>
> >>
> >>
> >> if (i == 0 && isThisReturn) {
> >> assert(!VA.needsCustom() && VA.getLocVT() == MVT::i64 &&
> >> "unexpected return calling convention register assignment");
> >> InVals.push_back(ThisVal);
> >> continue;
> >> }
> >>
> >>
> >> I’ve determined that the mask setting isn’t causing the issue, the
> >> above code is. But I’m not exactly sure why…
> >>
> >>
> >> Cheers,
> >>
> >>
> >> James
> >>
> >>
> >>
> >> On 11 Jul 2016, at 13:06, Hal Finkel < hfinkel at anl.gov > wrote:
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "James Molloy" < James.Molloy at arm.com >
> >> To: "Hal Finkel" < hfinkel at anl.gov >
> >> Cc: "Tim Northover" < t.p.northover at gmail.com >, "renato golin" <
> >> renato.golin at linaro.org >, llvm-commits at lists.llvm.org
> >> Sent: Monday, July 11, 2016 2:57:33 AM
> >> Subject: Re: [llvm] r275042 - Revert r275027 - Let FuncAttrs infer
> >> the 'returned' argument attribute
> >>
> >> Hi Hal,
> >>
> >> I’ll try and attack this today. I don’t suppose you noticed if any
> >> stage1 builders went awry too? That would help with getting it
> >> debugged quicker.
> >>
> >> Thanks! Unfortunately, no. Only the self-hosting builders were
> >> unhappy as far as I can tell.
> >>
> >> -Hal
> >>
> >>
> >>
> >>
> >> Cheers,
> >>
> >> James
> >>
> >>
> >> On 11 Jul 2016, at 06:11, Hal Finkel < hfinkel at anl.gov > wrote:
> >>
> >> Hi Tim, James, Renato,
> >>
> >> I'm not sure who is the best person to ask about this, but adding
> >> code to infer the 'returned' attribute on function arguments seems
> >> to cause the AArch64 backend to miscompile conspicuously:
> >>
> >> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/2564
> >>
> >> The other backends don't seem to have a problem, but then again,
> >> only ARM and AArch64 have special handling for 'returned'
> >> arguments. AArch64ISelLowering.cpp has, in part:
> >>
> >> if (VA.isRegLoc()) {
> >> if (realArgIdx == 0 && Flags.isReturned() && Outs[0].VT ==
> >> MVT::i64) {
> >> assert(VA.getLocVT() == MVT::i64 &&
> >> "unexpected calling convention register assignment");
> >> assert(!Ins.empty() && Ins[0].VT == MVT::i64 &&
> >> "unexpected use of 'returned'");
> >> ...
> >>
> >> and ARMISelLowering.cpp seems very similar.
> >>
> >> Any idea what's going on?
> >>
> >> To test, unrevert this commit (and r275043 for the few Clang test
> >> changes).
> >>
> >> Thanks again,
> >> Hal
> >>
> >> ----- Original Message -----
> >>
> >>
> >> From: "Hal Finkel via llvm-commits" <llvm-commits at lists.llvm.org>
> >> To: llvm-commits at lists.llvm.org
> >> Sent: Sunday, July 10, 2016 11:51:24 PM
> >> Subject: [llvm] r275042 - Revert r275027 - Let FuncAttrs infer the
> >> 'returned' argument attribute
> >>
> >> Author: hfinkel
> >> Date: Sun Jul 10 23:51:23 2016
> >> New Revision: 275042
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=275042&view=rev
> >> Log:
> >> Revert r275027 - Let FuncAttrs infer the 'returned' argument
> >> attribute
> >>
> >> Reverting r275027 and r275033. These seem to cause miscompiles on
> >> the
> >> AArch64 buildbot.
> >>
> >> Modified:
> >> llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
> >> llvm/trunk/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
> >> llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll
> >> llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll
> >>
> >> Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=275042&r1=275041&r2=275042&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp (original)
> >> +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Sun Jul 10
> >> 23:51:23 2016
> >> @@ -42,7 +42,6 @@ using namespace llvm;
> >> STATISTIC(NumReadNone, "Number of functions marked readnone");
> >> STATISTIC(NumReadOnly, "Number of functions marked readonly");
> >> STATISTIC(NumNoCapture, "Number of arguments marked nocapture");
> >> -STATISTIC(NumReturned, "Number of arguments marked returned");
> >> STATISTIC(NumReadNoneArg, "Number of arguments marked readnone");
> >> STATISTIC(NumReadOnlyArg, "Number of arguments marked readonly");
> >> STATISTIC(NumNoAlias, "Number of function returns marked
> >> noalias");
> >> @@ -484,53 +483,6 @@ determinePointerReadAttrs(Argument *A,
> >> return IsRead ? Attribute::ReadOnly : Attribute::ReadNone;
> >> }
> >>
> >> -/// Deduce returned attributes for the SCC.
> >> -static bool addArgumentReturnedAttrs(const SCCNodeSet &SCCNodes)
> >> {
> >> - bool Changed = false;
> >> -
> >> - AttrBuilder B;
> >> - B.addAttribute(Attribute::Returned);
> >> -
> >> - // Check each function in turn, determining if an argument is
> >> always returned.
> >> - for (Function *F : SCCNodes) {
> >> - // We can infer and propagate function attributes only when
> >> we
> >> know that the
> >> - // definition we'll get at link time is *exactly* the
> >> definition
> >> we see now.
> >> - // For more details, see GlobalValue::mayBeDerefined.
> >> - if (!F->hasExactDefinition())
> >> - continue;
> >> -
> >> - if (F->getReturnType()->isVoidTy())
> >> - continue;
> >> -
> >> - auto FindRetArg = [&]() -> Value * {
> >> - Value *RetArg = nullptr;
> >> - for (BasicBlock &BB : *F)
> >> - if (auto *Ret = dyn_cast<ReturnInst>(BB.getTerminator()))
> >> {
> >> - // Note that stripPointerCasts should look through
> >> functions with
> >> - // returned arguments.
> >> - Value *RetVal =
> >> Ret->getReturnValue()->stripPointerCasts();
> >> - if (RetVal->getType() == F->getReturnType() &&
> >> isa<Argument>(RetVal)) {
> >> - if (!RetArg)
> >> - RetArg = RetVal;
> >> - else if (RetArg != RetVal)
> >> - return nullptr;
> >> - }
> >> - }
> >> -
> >> - return RetArg;
> >> - };
> >> -
> >> - if (Value *RetArg = FindRetArg()) {
> >> - auto *A = cast<Argument>(RetArg);
> >> - A->addAttr(AttributeSet::get(F->getContext(), A->getArgNo()
> >> +
> >> 1, B));
> >> - ++NumReturned;
> >> - Changed = true;
> >> - }
> >> - }
> >> -
> >> - return Changed;
> >> -}
> >> -
> >> /// Deduce nocapture attributes for the SCC.
> >> static bool addArgumentAttrs(const SCCNodeSet &SCCNodes) {
> >> bool Changed = false;
> >> @@ -1072,7 +1024,6 @@ PreservedAnalyses PostOrderFunctionAttrs
> >> }
> >>
> >> bool Changed = false;
> >> - Changed |= addArgumentReturnedAttrs(SCCNodes);
> >> Changed |= addReadAttrs(SCCNodes, AARGetter);
> >> Changed |= addArgumentAttrs(SCCNodes);
> >>
> >> @@ -1138,7 +1089,6 @@ static bool runImpl(CallGraphSCC &SCC, A
> >> SCCNodes.insert(F);
> >> }
> >>
> >> - Changed |= addArgumentReturnedAttrs(SCCNodes);
> >> Changed |= addReadAttrs(SCCNodes, AARGetter);
> >> Changed |= addArgumentAttrs(SCCNodes);
> >>
> >>
> >> Modified:
> >> llvm/trunk/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll?rev=275042&r1=275041&r2=275042&view=diff
> >> ==============================================================================
> >> ---
> >> llvm/trunk/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
> >> (original)
> >> +++
> >> llvm/trunk/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
> >> Sun Jul 10 23:51:23 2016
> >> @@ -14,7 +14,7 @@ define i32* @b(i32 *%q) {
> >> ret i32* %tmp
> >> }
> >>
> >> -; CHECK: define i32* @c(i32* readnone returned %r)
> >> +; CHECK: define i32* @c(i32* readnone %r)
> >> @g = global i32 0
> >> define i32* @c(i32 *%r) {
> >> %a = icmp eq i32* %r, null
> >>
> >> Modified: llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll?rev=275042&r1=275041&r2=275042&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll
> >> (original)
> >> +++ llvm/trunk/test/Transforms/FunctionAttrs/nocapture.ll Sun Jul
> >> 10
> >> 23:51:23 2016
> >> @@ -1,7 +1,7 @@
> >> ; RUN: opt < %s -functionattrs -S | FileCheck %s
> >> @g = global i32* null; <i32**> [#uses=1]
> >>
> >> -; CHECK: define i32* @c1(i32* readnone returned %q)
> >> +; CHECK: define i32* @c1(i32* readnone %q)
> >> define i32* @c1(i32* %q) {
> >> ret i32* %q
> >> }
> >> @@ -140,7 +140,7 @@ define void @test1_1(i8* %x1_1, i8* %y1_
> >> ret void
> >> }
> >>
> >> -; CHECK: define i8* @test1_2(i8* nocapture readnone %x1_2, i8*
> >> returned %y1_2)
> >> +; CHECK: define i8* @test1_2(i8* nocapture readnone %x1_2, i8*
> >> %y1_2)
> >> define i8* @test1_2(i8* %x1_2, i8* %y1_2) {
> >> call void @test1_1(i8* %x1_2, i8* %y1_2)
> >> store i32* null, i32** @g
> >> @@ -168,7 +168,7 @@ define void @test4_1(i8* %x4_1) {
> >> ret void
> >> }
> >>
> >> -; CHECK: define i8* @test4_2(i8* nocapture readnone %x4_2, i8*
> >> readnone returned %y4_2, i8* nocapture readnone %z4_2)
> >> +; CHECK: define i8* @test4_2(i8* nocapture readnone %x4_2, i8*
> >> readnone %y4_2, i8* nocapture readnone %z4_2)
> >> define i8* @test4_2(i8* %x4_2, i8* %y4_2, i8* %z4_2) {
> >> call void @test4_1(i8* null)
> >> store i32* null, i32** @g
> >>
> >> Modified: llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll?rev=275042&r1=275041&r2=275042&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll
> >> (original)
> >> +++ llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll Sun Jul
> >> 10
> >> 23:51:23 2016
> >> @@ -11,7 +11,7 @@ define void @test1_2(i8* %x1_2, i8* %y1_
> >> ret void
> >> }
> >>
> >> -; CHECK: define i8* @test2(i8* readnone returned %p)
> >> +; CHECK: define i8* @test2(i8* readnone %p)
> >> define i8* @test2(i8* %p) {
> >> store i32 0, i32* @x
> >> ret i8* %p
> >> @@ -53,7 +53,7 @@ define void @test7_1(i32* inalloca %a) {
> >> ret void
> >> }
> >>
> >> -; CHECK: define i32* @test8_1(i32* readnone returned %p)
> >> +; CHECK: define i32* @test8_1(i32* readnone %p)
> >> define i32* @test8_1(i32* %p) {
> >> entry:
> >> ret i32* %p
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >>
> >>
> >> IMPORTANT NOTICE: The contents of this email and any attachments
> >> are
> >> confidential and may also be privileged. If you are not the
> >> intended
> >> recipient, please notify the sender immediately and do not
> >> disclose
> >> the contents to any other person, use it for any purpose, or store
> >> or copy the information in any medium. Thank you.
> >>
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >> IMPORTANT NOTICE: The contents of this email and any attachments
> >> are
> >> confidential and may also be privileged. If you are not the
> >> intended
> >> recipient, please notify the sender immediately and do not
> >> disclose
> >> the contents to any other person, use it for any purpose, or store
> >> or copy the information in any medium. Thank you.
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> >
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose
> the contents to any other person, use it for any purpose, or store
> or copy the information in any medium. Thank you.
>
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list