[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 07:05:30 PDT 2016


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<mailto:hfinkel at anl.gov>> wrote:

----- Original Message -----
From: "James Molloy" <James.Molloy at arm.com<mailto:James.Molloy at arm.com>>
To: "Hal Finkel" <hfinkel at anl.gov<mailto:hfinkel at anl.gov>>
Cc: "Tim Northover" <t.p.northover at gmail.com<mailto:t.p.northover at gmail.com>>, "renato golin" <renato.golin at linaro.org<mailto:renato.golin at linaro.org>>, llvm-commits at lists.llvm.org<mailto: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<mailto: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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160711/cc972ce8/attachment.html>


More information about the llvm-commits mailing list