[llvm] r275042 - Revert r275027 - Let FuncAttrs infer the 'returned' argument attribute

Fri Jul 15 10:16:45 PDT 2016

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