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

James Molloy via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 11 00:57:33 PDT 2016


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.

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.


More information about the llvm-commits mailing list