[FunctionAttrs] Unconditionally perform argument attribute inference in the first function-attrs pass

[cfang]

Summary

Argument attributes like NoAlias and ReadOnly could affect memoryssa and thus earlyCSE in the function simplification pipeline.
https://reviews.llvm.org/D145210 adjusted PostOrderFunctionAttrs placement and caused the argument attributes not referred for the use
in the pipeline. This work (initiated by @nikic) unconditionally performs argument attribute inference in the first function-attrs pass.

Based on @nikic's experiment, the compile-time impact is pretty low: http://llvm-compile-time-tracker.com/compare.php?from=74258f4189e2b6bacabd40ee6f588fd9d1b37741&to=7f7484aee497cdae817b4aa24aed8046f430a816&stat=instructions%3Au

[nikic]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

1769

Add some extra comment here: "Only infer readonly etc on arguments in that case, because it can affect optimization behavior in conjunction with noalias" or something like that.

llvm/test/Transforms/PhaseOrdering/arg-attrs-affect-earlycse.ll

174 ↗

(On Diff #544623)

It should be possible to massively simplify this test by using the memssa-check-limit option.

Please drop all the irrelevant noise, especially anything related to address spaces.

[cfang]

Add extra comment as suggested, and further reduce the test case. Thanks.

[aeubanks]

llvm/test/Transforms/PhaseOrdering/arg-attrs-affect-earlycse.ll

1 ↗

(On Diff #544918)

nit: extra space before RUN

3 ↗

(On Diff #544918)

I don't think it's a good idea in these phase ordering tests to specifically check that a specific pass did something, future pipeline changes can break this; the whole point of these is that the entire pipeline gives a good result regardless of exactly what's happening in the pipelines. This should use llvm/utils/update_test_checks.py like the other phase ordering tests to check the final IR. Although I tried that for this test and this patch doesn't change it, so I think you'll need to change this test to get something where this patch actually improves something. Testing with -memssa-check-limit=1 is fine though.

6 ↗

(On Diff #544918)

these function properties can be removed

48 ↗

(On Diff #544918)

can be a declaration

53 ↗

(On Diff #544918)

function attributes can be dropped

[cfang]

llvm/test/Transforms/PhaseOrdering/arg-attrs-affect-earlycse.ll

3 ↗

(On Diff #544918)

We could not successfully extract a small test from the huge application that can show the difference after the pipeline.
Would it be fine to check the IR after early-cse<memssa>? Thanks. and appreciate the the help and suggestions.

[cfang]

Update the test based on the suggestions.

[cfang]

Is there any further suggestion on the test?
Currently we are using "update_test_checks.py" to generate checks automatically that can show the difference before and after the patch.
I have the note in the test to express our interests. This seems the best I can do at this moment. Thanks.

[aeubanks]

sorry for the late response

as I keep saying, the test is still not really a phase ordering test because it's not exercising the entire pipeline, you're printing the IR very early on in the pipeline, but you should be printing the IR at the very end of the pipeline: opt -O3 -S < %s. please find a test that is a proper phase ordering test and also is affected by this func-attrs change

[aeubanks]

the example you gave in D145210 and the test added here both are ultimately unaffected by this patch, if you look at the entire -O3 pipeline with and without this patch

[aeubanks]

here's a test case that this patch fixes

; RUN: opt -S -O3 -memssa-check-limit=1 -memdep-block-scan-limit=1 < %s | FileCheck %s
declare void @g()

define i32 @f(ptr noalias %p, i32 %c) {
  %i = load i32, ptr %p
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  call void @g()
  %i2 = load i32, ptr %p
  %r = sub i32 %i, %i2
  ret i32 %r
}

-memdep-block-scan-limit is the equivalent of -memssa-check-limit for memdep analysis which GVN still uses

[cfang]

Update the LIT test with the example @aeubanks developed. Thanks.

[nikic]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

7340 ↗

(On Diff #548315)

Debugging leftover?

llvm/test/Transforms/PhaseOrdering/early-arg-attrs-inference.ll

3

Please pre-commit the test.

[cfang]

Remove an accidental return in the code.

[aeubanks]

last nit then I think this is good to go
function-attrs<skip-non-recursive> should be renamed to something like function-attrs<non-recursive-only-arg-attrs>

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

7340 ↗

(On Diff #548315)

unintenitonal change?

[cfang]

In D156397#4570449, @aeubanks wrote:

last nit then I think this is good to go
function-attrs<skip-non-recursive> should be renamed to something like function-attrs<non-recursive-only-arg-attrs>

Should it be function-attrs<recursive-or-arg-attrs-only> ?

[aeubanks]

In D156397#4570516, @cfang wrote:

In D156397#4570449, @aeubanks wrote:

last nit then I think this is good to go
function-attrs<skip-non-recursive> should be renamed to something like function-attrs<non-recursive-only-arg-attrs>

Should it be function-attrs<recursive-or-arg-attrs-only> ?

The "or" is very declarative as opposed to imperative, which I think is more confusing. I imagine function-attrs as performing everything, then <foo> is a constraint on top of that, so something like <skip-non-recursive-function-attrs> makes more sense to me.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

1769

I'd remove this comment and change below to Only infer argument attributes for non-recursive functions, because...

[aeubanks]

in terms of the request to pre-commit the test, split out the creation of the test into a separate patch (with update_test_checks.py run on it with ToT opt), then rebase this patch on top of that with update_test_checks.py reran with these changes, and then in this patch we can the diff on the test case that this patch causes

(basically the workflow explained in the comments in llvm/utils/update_test_checks.py)

[cfang]

> The "or" is very declarative as opposed to imperative, which I think is more confusing. I imagine function-attrs as performing everything, then <foo> is a constraint on top of that, so something like <skip-non-recursive-function-attrs> makes more sense to me.

Do you mean to replace "skip-non-recursive" with "skip-non-recursive-function-attrs"? (this does not say anything about argument attribute explicitly)
Do you also want the variable name and related comments to be changed?
"bool SkipNonRecursive" for example?

[cfang]

update a comment
add pre-commit of the LIT to show the diff from this patch.

[cfang]

Rename to "skip-non-recursive-function-attrs" as suggested

[aeubanks]

lgtm

llvm/lib/Passes/PassBuilder.cpp

Expected<bool> parseInlinerPassOptions(StringRef Params) {
   return parseSinglePassOption(Params, "only-mandatory", "InlinerPass");
 }
 
 Expected<bool> parseCoroSplitPassOptions(StringRef Params) {
   return parseSinglePassOption(Params, "reuse-storage", "CoroSplitPass");
 }
 
 Expected<bool> parsePostOrderFunctionAttrsPassOptions(StringRef Params) {
-  return parseSinglePassOption(Params, "skip-non-recursive",
+  return parseSinglePassOption(Params, "skip-non-recursive-function-attrs",
                                "PostOrderFunctionAttrs");
 }
 
 Expected<bool> parseEarlyCSEPassOptions(StringRef Params) {
   return parseSinglePassOption(Params, "memssa", "EarlyCSE");
 }
 
 Expected<bool> parseEntryExitInstrumenterPassOptions(StringRef Params) {

llvm/lib/Passes/PassRegistry.def

CGSCC_PASS_WITH_PARAMS("coro-split",
                        parseCoroSplitPassOptions,
                        "reuse-storage")
 CGSCC_PASS_WITH_PARAMS("function-attrs",
                        "PostOrderFunctionAttrsPass",
                        [](bool SkipNonRecursive) {
                          return PostOrderFunctionAttrsPass(SkipNonRecursive);
                        },
                        parsePostOrderFunctionAttrsPassOptions,
-                       "skip-non-recursive")
+                       "skip-non-recursive-function-attrs")
 #undef CGSCC_PASS_WITH_PARAMS
 
 #ifndef FUNCTION_ANALYSIS
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)
 #endif
 FUNCTION_ANALYSIS("aa", AAManager())
 FUNCTION_ANALYSIS("assumptions", AssumptionAnalysis())
 FUNCTION_ANALYSIS("block-freq", BlockFrequencyAnalysis())

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

for (Function *F : Functions) {
     }
     Res.SCCNodes.insert(F);
   }
   return Res;
 }
 
 template <typename AARGetterT>
 static SmallSet<Function *, 8>
-deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter) {
+deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter,
+                       bool ArgAttrsOnly) {
   SCCNodesResult Nodes = createSCCNodeSet(Functions);
 
   // Bail if the SCC only contains optnone functions.
   if (Nodes.SCCNodes.empty())
     return {};
 
   SmallSet<Function *, 8> Changed;
+  if (ArgAttrsOnly) {
+    addArgumentAttrs(Nodes.SCCNodes, Changed);
+    return Changed;
+  }
 
   addArgumentReturnedAttrs(Nodes.SCCNodes, Changed);
   addMemoryAttrs(Nodes.SCCNodes, AARGetter, Changed);
   addArgumentAttrs(Nodes.SCCNodes, Changed);
   inferConvergent(Nodes.SCCNodes, Changed);
   addNoReturnAttrs(Nodes.SCCNodes, Changed);
   addWillReturn(Nodes.SCCNodes, Changed);
 
deriveAttrsInPostOrder(ArrayRef<Function *> Functions, AARGetterT &&AARGetter,
 
   return Changed;
 }
 
 PreservedAnalyses PostOrderFunctionAttrsPass::run(LazyCallGraph::SCC &C,
                                                   CGSCCAnalysisManager &AM,
                                                   LazyCallGraph &CG,
                                                   CGSCCUpdateResult &) {
   // Skip non-recursive functions if requested.
+  // Only infer argument attributes for non-recursive functions, because
+  // it can affect optimization behavior in conjunction with noalias.
+  bool ArgAttrsOnly = false;
   if (C.size() == 1 && SkipNonRecursive) {
     LazyCallGraph::Node &N = *C.begin();
     if (!N->lookup(N))
-      return PreservedAnalyses::all();
+      ArgAttrsOnly = true;
   }
 
   FunctionAnalysisManager &FAM =
       AM.getResult<FunctionAnalysisManagerCGSCCProxy>(C, CG).getManager();
 
   // We pass a lambda into functions to wire them up to the analysis manager
   // for getting function analyses.
   auto AARGetter = [&](Function &F) -> AAResults & {
     return FAM.getResult<AAManager>(F);
   };
 
   SmallVector<Function *, 8> Functions;
   for (LazyCallGraph::Node &N : C) {
     Functions.push_back(&N.getFunction());
   }
 
-  auto ChangedFunctions = deriveAttrsInPostOrder(Functions, AARGetter);
+  auto ChangedFunctions =
+      deriveAttrsInPostOrder(Functions, AARGetter, ArgAttrsOnly);
   if (ChangedFunctions.empty())
     return PreservedAnalyses::all();
 
   // Invalidate analyses for modified functions so that we don't have to
   // invalidate all analyses for all functions in this SCC.
   PreservedAnalyses FuncPA;
   // We haven't changed the CFG for modified functions.
   FuncPA.preserveSet<CFGAnalyses>();
PreservedAnalyses PostOrderFunctionAttrsPass::run(LazyCallGraph::SCC &C,
   return PA;
 }
 
 void PostOrderFunctionAttrsPass::printPipeline(
     raw_ostream &OS, function_ref<StringRef(StringRef)> MapClassName2PassName) {
   static_cast<PassInfoMixin<PostOrderFunctionAttrsPass> *>(this)->printPipeline(
       OS, MapClassName2PassName);
   if (SkipNonRecursive)
-    OS << "<skip-non-recursive>";
+    OS << "<skip-non-recursive-function-attrs>";
 }
 
 template <typename AARGetterT>
 static bool runImpl(CallGraphSCC &SCC, AARGetterT AARGetter) {
   SmallVector<Function *, 8> Functions;
   for (CallGraphNode *I : SCC) {
     Functions.push_back(I->getFunction());
   }

llvm/test/Other/new-pm-print-pipeline.ll

 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='separate-const-offset-from-gep<lower-gep>' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-27
 ; CHECK-27: function(separate-const-offset-from-gep<lower-gep>)
 
 ;; Test InstCombine options - the first pass checks default settings, and the second checks customized options.
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='function(instcombine,instcombine<use-loop-info;no-verify-fixpoint;max-iterations=42>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-28
 ; CHECK-28: function(instcombine<max-iterations=1;no-use-loop-info;verify-fixpoint>,instcombine<max-iterations=42;use-loop-info;no-verify-fixpoint>)
 
 ;; Test function-attrs
-; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='cgscc(function-attrs<skip-non-recursive>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-29
-; CHECK-29: cgscc(function-attrs<skip-non-recursive>)
+; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='cgscc(function-attrs<skip-non-recursive-function-attrs>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-29
+; CHECK-29: cgscc(function-attrs<skip-non-recursive-function-attrs>)
 
 ;; Test cgscc -> function adaptor
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='cgscc(function<eager-inv;no-rerun>(no-op-function))' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-30
 ; CHECK-30: cgscc(function<eager-inv;no-rerun>(no-op-function))
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='cgscc(function<no-rerun;eager-inv>(no-op-function))' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-31
 ; CHECK-31: cgscc(function<eager-inv;no-rerun>(no-op-function))
 

llvm/test/Transforms/InstCombine/unused-nonnull.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
 ; RUN: opt -S -O3 -o - %s | FileCheck %s
 
 ; PR44154: LLVM c3b06d0c393e caused the body of @main to be replaced with
 ; unreachable. Check that we perform the expected calls and optimizations.
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define i32 @main(i32 %argc, ptr %argv) #0 {
 ; CHECK-LABEL: define {{[^@]+}}@main
-; CHECK-SAME: (i32 [[ARGC:%.*]], ptr nocapture readnone [[ARGV:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+; CHECK-SAME: (i32 [[ARGC:%.*]], ptr nocapture readonly [[ARGV:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = icmp slt i32 [[ARGC]], 2
 ; CHECK-NEXT:    [[SPEC_SELECT:%.*]] = select i1 [[TMP0]], i32 0, i32 [[ARGC]]
 ; CHECK-NEXT:    ret i32 [[SPEC_SELECT]]
 ;
 entry:
   %ptr = load ptr, ptr %argv
   %0 = call i32 @compute(ptr %ptr, i32 %argc)

llvm/test/Transforms/PhaseOrdering/early-arg-attrs-inference.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S -O3 -memssa-check-limit=1 -memdep-block-scan-limit=1 < %s | FileCheck %s
+
+define i32 @f(ptr noalias %p, i32 %c) {
+; CHECK-LABEL: define i32 @f
+; CHECK-SAME: (ptr noalias nocapture readonly [[P:%.*]], i32 [[C:%.*]]) local_unnamed_addr {
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    tail call void @g()
+; CHECK-NEXT:    ret i32 0
+;
+  %i = load i32, ptr %p
+  call void @g()
+  call void @g()
+  call void @g()
+  call void @g()
+  call void @g()
+  call void @g()
+  %i2 = load i32, ptr %p
+  %r = sub i32 %i, %i2
+  ret i32 %r
+}
+
+declare void @g()