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

Fri Jul 15 10:21:06 PDT 2016

Hi Hal,

That seems reasonable to me.

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.