[PATCH] D128830: [Pipelines] Introduce DAE after ArgumentPromotion

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

moving DAE after the function simplification pipeline makes sense

In D128830#3622736 <https://reviews.llvm.org/D128830#3622736>, @psamolysov wrote:

> In D128830#3622467 <https://reviews.llvm.org/D128830#3622467>, @fhahn wrote:
>
>> Do we need to retain the run of `DeadArgumentEliminationPass` in the original position or is a single run at the new position sufficient?
>
> Good point! I tried and also removed the guard that the DAE pass should run with O3 <https://reviews.llvm.org/owners/package/3/> only (currently it run exactly as the pass in the original position did: with any O > 0). I have a chance to run the LLVM :: Transforms tests only, one test failed - `LLVM :: Transforms\InstCombine\unused-nonnull.ll`:
>
>   error: CHECK-SAME: expected string not found in input
>   ; CHECK-SAME: (i32 [[ARGC:%.*]], i8** nocapture readnone [[ARGV:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
>                 ^
>   <stdin>:7:17: note: scanning from here
>   define i32 @main(i32 %argc, i8** nocapture readonly %argv) local_unnamed_addr #0 {
>
> The difference is that the `%argv` argument has the `readonly` attribute, not `readnone`. I'm not sure whether this makes much sense (I guess this  could because `readnone` can theoretically open a door for some optimizations for which `readonly` cannot).

this would be fixed by running `PostOrderFunctionAttrsPass` after the function simplification pipeline which is something I've wanted to do but never got around to testing. we should be inferring more precise attributes after fully simplifying functions. the only case that might regress is recursive functions since when visiting the recursive calls we haven't computed attributes for the recursive functions yet

this test is regressing with this patch because previously DAE would replace passed arguments with poison if the argument isn't used in the function, removing the use of `%argv` in `@main` and func-attrs would mark `%argv` as readnone, but with this patch func-attrs runs on `@main` before the use of `%argv` is eliminated. the call to `@compute` is eliminated in all cases in the function simplification pipeline due to inferring the `returned` attribute on `%x`, which is why running func-attrs after the function simplification pipeline fixes this issue

  @@ -759,9 +758,6 @@ PassBuilder::buildInlinerPipeline(OptimizationLevel Level,
     if (AttributorRun & AttributorRunOption::CGSCC)
       MainCGPipeline.addPass(AttributorCGSCCPass());
   
  -  // Now deduce any function attributes based in the current code.
  -  MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
  -
     // When at O3 add argument promotion to the pass pipeline.
     // FIXME: It isn't at all clear why this should be limited to O3.
     if (Level == OptimizationLevel::O3)
  @@ -781,6 +777,9 @@ PassBuilder::buildInlinerPipeline(OptimizationLevel Level,
         buildFunctionSimplificationPipeline(Level, Phase),
         PTO.EagerlyInvalidateAnalyses, EnableNoRerunSimplificationPipeline));
   
  +  // Now deduce any function attributes based in the current code.
  +  MainCGPipeline.addPass(PostOrderFunctionAttrsPass());
  +
     MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128830/new/

https://reviews.llvm.org/D128830