[llvm] ab98f2c - Revert "[InferAttributes] Materialize all infered attributes for declaration"

Wed Apr 14 16:40:47 PDT 2021

For those following along...

This has been reapplied in dd985551.  For some reason, the original 
phabricator link Nico posted didn't make it into an email to me.  A big 
chunk of the confusion was that there were two similarly named tests 
(one llvm, one clang) with identical failure messages, but each bot only 
reported one.  (Odd...)  As a result, I didn't notice the second issue.

Nico, thanks for the revert.  Definitely warranted in this case.

Philip

On 4/14/21 3:46 PM, Philip Reames via llvm-commits wrote:
> Nico,
>
> Can you point me to the builder which was still failing?  I'd missed 
> an AMDGPU test, but that was fixed before your revert.  (I think your 
> revert actually left tree in a broken state since you don't revert 
> that change btw.)
>
> Philip
>
> On 4/14/21 3:42 PM, Nico Weber via llvm-commits wrote:
>> Author: Nico Weber
>> Date: 2021-04-14T18:41:20-04:00
>> New Revision: ab98f2c7129a52e216fd7e088b964cf4af27b0f2
>>
>> URL: 
>> https://github.com/llvm/llvm-project/commit/ab98f2c7129a52e216fd7e088b964cf4af27b0f2
>> DIFF: 
>> https://github.com/llvm/llvm-project/commit/ab98f2c7129a52e216fd7e088b964cf4af27b0f2.diff
>>
>> LOG: Revert "[InferAttributes] Materialize all infered attributes for 
>> declaration"
>>
>> Breaks check-clang, see comments on D100400
>>
>> Also revert follow-up "[NFC] Move a recently added utility into a 
>> location to enable reuse"
>>
>> This reverts commit 3ce61fb6d697d49db471c7077b88b3b9ec9dec66.
>> This reverts commit 61a85da88235983da565bda0160367461fa0f382.
>>
>> Added:
>>
>> Modified:
>>      llvm/include/llvm/Transforms/Utils/Local.h
>>      llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp
>>      llvm/lib/Transforms/Utils/Local.cpp
>>      llvm/test/Other/cgscc-devirt-iteration.ll
>>      llvm/test/Transforms/InferFunctionAttrs/annotate.ll
>>      llvm/test/Transforms/LICM/strlen.ll
>>
>> Removed:
>>
>>
>> ################################################################################ 
>>
>> diff  --git a/llvm/include/llvm/Transforms/Utils/Local.h 
>> b/llvm/include/llvm/Transforms/Utils/Local.h
>> index 8ab066f0b6a3..f7efeeb56fd3 100644
>> --- a/llvm/include/llvm/Transforms/Utils/Local.h
>> +++ b/llvm/include/llvm/Transforms/Utils/Local.h
>> @@ -488,15 +488,6 @@ bool canReplaceOperandWithVariable(const 
>> Instruction *I, unsigned OpIdx);
>>   /// Invert the given true/false value, possibly reusing an existing 
>> copy.
>>   Value *invertCondition(Value *Condition);
>>   -
>> -//===----------------------------------------------------------------------===// 
>>
>> -//  Assorted
>> -//
>> -
>> -/// If we can infer one attribute from another on the declaration of a
>> -/// function, explicitly materialize the maximal set in the IR.
>> -bool inferAttributesFromOthers(Function &F);
>> -
>>   } // end namespace llvm
>>     #endif // LLVM_TRANSFORMS_UTILS_LOCAL_H
>>
>> diff  --git a/llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp 
>> b/llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp
>> index 30402f109f30..685f8f7d7a00 100644
>> --- a/llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp
>> +++ b/llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp
>> @@ -15,7 +15,6 @@
>>   #include "llvm/Support/Debug.h"
>>   #include "llvm/Support/raw_ostream.h"
>>   #include "llvm/Transforms/Utils/BuildLibCalls.h"
>> -#include "llvm/Transforms/Utils/Local.h"
>>   using namespace llvm;
>>     #define DEBUG_TYPE "inferattrs"
>> @@ -26,14 +25,9 @@ static bool inferAllPrototypeAttributes(
>>       for (Function &F : M.functions())
>>       // We only infer things using the prototype and the name; we 
>> don't need
>> -    // definitions.  This ensures libfuncs are annotated and also 
>> allows our
>> -    // CGSCC inference to avoid needing to duplicate the inference 
>> from other
>> -    // attribute logic on all calls to declarations (as declarations 
>> aren't
>> -    // explicitly visited by CGSCC passes in the new pass manager.)
>> -    if (F.isDeclaration() && !F.hasOptNone()) {
>> +    // definitions.
>> +    if (F.isDeclaration() && !F.hasOptNone())
>>         Changed |= inferLibFuncAttributes(F, GetTLI(F));
>> -      Changed |= inferAttributesFromOthers(F);
>> -    }
>>       return Changed;
>>   }
>>
>> diff  --git a/llvm/lib/Transforms/Utils/Local.cpp 
>> b/llvm/lib/Transforms/Utils/Local.cpp
>> index 89e62fb94ec1..e285f8aa7a20 100644
>> --- a/llvm/lib/Transforms/Utils/Local.cpp
>> +++ b/llvm/lib/Transforms/Utils/Local.cpp
>> @@ -3392,33 +3392,3 @@ Value *llvm::invertCondition(Value *Condition) {
>> Inverted->insertBefore(&*Parent->getFirstInsertionPt());
>>     return Inverted;
>>   }
>> -
>> -bool llvm::inferAttributesFromOthers(Function &F) {
>> -  // Note: We explicitly check for attributes rather than using 
>> cover functions
>> -  // because some of the cover functions include the logic being 
>> implemented.
>> -
>> -  bool Changed = false;
>> -  // readnone + not convergent implies nosync
>> -  if (!F.hasFnAttribute(Attribute::NoSync) &&
>> -      F.doesNotAccessMemory() && !F.isConvergent()) {
>> -    F.setNoSync();
>> -    Changed = true;
>> -  }
>> -
>> -  // readonly implies nofree
>> -  if (!F.hasFnAttribute(Attribute::NoFree) && F.onlyReadsMemory()) {
>> -    F.setDoesNotFreeMemory();
>> -    Changed = true;
>> -  }
>> -
>> -  // willreturn implies mustprogress
>> -  if (!F.hasFnAttribute(Attribute::MustProgress) && F.willReturn()) {
>> -    F.setMustProgress();
>> -    Changed = true;
>> -  }
>> -
>> -  // TODO: There are a bunch of cases of restrictive memory effects we
>> -  // can infer by inspecting arguments of argmemonly-ish functions.
>> -
>> -  return Changed;
>> -}
>>
>> diff  --git a/llvm/test/Other/cgscc-devirt-iteration.ll 
>> b/llvm/test/Other/cgscc-devirt-iteration.ll
>> index 092c624442db..651b7f3b2853 100644
>> --- a/llvm/test/Other/cgscc-devirt-iteration.ll
>> +++ b/llvm/test/Other/cgscc-devirt-iteration.ll
>> @@ -4,18 +4,18 @@
>>   ; devirtualization here with GVN which forwards a store through a 
>> load and to
>>   ; an indirect call.
>>   ;
>> -; RUN: opt -aa-pipeline=basic-aa 
>> -passes='module(inferattrs),cgscc(function-attrs,function(gvn,instcombine))' 
>> -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=BEFORE
>> -; RUN: opt -aa-pipeline=basic-aa 
>> -passes='module(inferattrs),cgscc(devirt<1>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER 
>> --check-prefix=AFTER1
>> -; RUN: opt -aa-pipeline=basic-aa 
>> -passes='module(inferattrs),cgscc(devirt<2>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER 
>> --check-prefix=AFTER2
>> +; RUN: opt -aa-pipeline=basic-aa 
>> -passes='cgscc(function-attrs,function(gvn,instcombine))' -S < %s | 
>> FileCheck %s --check-prefix=CHECK --check-prefix=BEFORE
>> +; RUN: opt -aa-pipeline=basic-aa 
>> -passes='cgscc(devirt<1>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER 
>> --check-prefix=AFTER1
>> +; RUN: opt -aa-pipeline=basic-aa 
>> -passes='cgscc(devirt<2>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s | FileCheck %s --check-prefix=CHECK --check-prefix=AFTER 
>> --check-prefix=AFTER2
>>   ;
>> -; RUN: not --crash opt -abort-on-max-devirt-iterations-reached 
>> -aa-pipeline=basic-aa 
>> -passes='module(inferattrs),cgscc(devirt<1>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s
>> -; RUN: opt -abort-on-max-devirt-iterations-reached 
>> -aa-pipeline=basic-aa 
>> -passes='module(inferattrs),cgscc(devirt<2>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s
>> +; RUN: not --crash opt -abort-on-max-devirt-iterations-reached 
>> -aa-pipeline=basic-aa 
>> -passes='cgscc(devirt<1>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s
>> +; RUN: opt -abort-on-max-devirt-iterations-reached 
>> -aa-pipeline=basic-aa 
>> -passes='cgscc(devirt<2>(function-attrs,function(gvn,instcombine)))' 
>> -S < %s
>>   ;
>>   ; We also verify that the real O2 pipeline catches these cases.
>>   ; RUN: opt -aa-pipeline=basic-aa -passes='default<O2>' -S < %s | 
>> FileCheck %s --check-prefix=CHECK --check-prefix=AFTER 
>> --check-prefix=AFTER2
>>     declare void @readnone() readnone
>> -; CHECK: Function Attrs: nofree nosync readnone
>> +; CHECK: Function Attrs: readnone
>>   ; CHECK-NEXT: declare void @readnone()
>>     declare void @unknown()
>> @@ -51,7 +51,7 @@ entry:
>>   ; devirtualize again, and then deduce readnone.
>>     declare void @readnone_with_arg(void ()**) readnone
>> -; CHECK: Function Attrs: nofree nosync readnone
>> +; CHECK: Function Attrs: readnone
>>   ; CHECK-LABEL: declare void @readnone_with_arg(void ()**)
>>     define void @test2_a(void ()** %ignore) {
>>
>> diff  --git a/llvm/test/Transforms/InferFunctionAttrs/annotate.ll 
>> b/llvm/test/Transforms/InferFunctionAttrs/annotate.ll
>> index 05cf79b3874b..c71688ba3d15 100644
>> --- a/llvm/test/Transforms/InferFunctionAttrs/annotate.ll
>> +++ b/llvm/test/Transforms/InferFunctionAttrs/annotate.ll
>> @@ -1016,18 +1016,18 @@ declare i64 @write(i32, i8*, i64)
>>   declare void @memset_pattern16(i8*, i8*, i64)
>>     -; CHECK-DAG: attributes [[NOFREE_NOUNWIND_WILLRETURN]] = { 
>> nofree nounwind willreturn mustprogress }
>> +; CHECK-DAG: attributes [[NOFREE_NOUNWIND_WILLRETURN]] = { nofree 
>> nounwind willreturn }
>>   ; CHECK-DAG: attributes [[NOFREE_NOUNWIND]] = { nofree nounwind }
>> -; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = { 
>> inaccessiblememonly nofree nounwind willreturn mustprogress }
>> -; CHECK-DAG: attributes [[NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { 
>> nofree nounwind readonly willreturn mustprogress }
>> -; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = 
>> { argmemonly nofree nounwind willreturn mustprogress }
>> +; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = { 
>> inaccessiblememonly nofree nounwind willreturn }
>> +; CHECK-DAG: attributes [[NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { 
>> nofree nounwind readonly willreturn }
>> +; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = 
>> { argmemonly nofree nounwind willreturn }
>>   ; CHECK-DAG: attributes [[NOFREE_NOUNWIND_READONLY]] = { nofree 
>> nounwind readonly }
>> -; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMORARGMEMONLY_NOUNWIND_WILLRETURN]] = { 
>> inaccessiblemem_or_argmemonly nounwind willreturn mustprogress }
>> -; CHECK-DAG: attributes [[NOFREE_WILLRETURN]] = { nofree willreturn 
>> mustprogress }
>> -; CHECK-DAG: attributes 
>> [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { argmemonly 
>> nofree nounwind readonly willreturn mustprogress }
>> +; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMORARGMEMONLY_NOUNWIND_WILLRETURN]] = { 
>> inaccessiblemem_or_argmemonly nounwind willreturn }
>> +; CHECK-DAG: attributes [[NOFREE_WILLRETURN]] = { nofree willreturn }
>> +; CHECK-DAG: attributes 
>> [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { argmemonly 
>> nofree nounwind readonly willreturn }
>>   ; CHECK-DAG: attributes [[NOFREE]] = { nofree }
>> -; CHECK-DAG: attributes [[WILLRETURN]] = { willreturn mustprogress }
>> -; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMORARGONLY_NOFREE_NOUNWIND_WILLRETURN]]  = { 
>> inaccessiblemem_or_argmemonly nofree nounwind willreturn mustprogress }
>> +; CHECK-DAG: attributes [[WILLRETURN]] = { willreturn }
>> +; CHECK-DAG: attributes 
>> [[INACCESSIBLEMEMORARGONLY_NOFREE_NOUNWIND_WILLRETURN]]  = { 
>> inaccessiblemem_or_argmemonly nofree nounwind willreturn }
>>     ; CHECK-DARWIN-DAG: attributes [[ARGMEMONLY_NOFREE]] = { 
>> argmemonly nofree }
>> -; CHECK-NVPTX-DAG: attributes [[NOFREE_NOUNWIND_READNONE]] = { 
>> nofree nosync nounwind readnone }
>> +; CHECK-NVPTX-DAG: attributes [[NOFREE_NOUNWIND_READNONE]] = { 
>> nofree nounwind readnone }
>>
>> diff  --git a/llvm/test/Transforms/LICM/strlen.ll 
>> b/llvm/test/Transforms/LICM/strlen.ll
>> index 4a45a06f82f7..3f54eecafd80 100644
>> --- a/llvm/test/Transforms/LICM/strlen.ll
>> +++ b/llvm/test/Transforms/LICM/strlen.ll
>> @@ -13,7 +13,7 @@ loop:
>>   }
>>     ; CHECK: declare i64 @strlen(i8* nocapture) #0
>> -; CHECK: attributes #0 = { argmemonly nofree nounwind readonly 
>> willreturn mustprogress }
>> +; CHECK: attributes #0 = { argmemonly nofree nounwind readonly 
>> willreturn }
>>   declare i64 @strlen(i8*)
>>
>>
>>          _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits