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

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added a comment.

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).

I've submitted this change to let the buildbot run all the tests and see the difference.



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:775
+  if (Level == OptimizationLevel::O3)
+    MIWP.addLateModulePass(DeadArgumentEliminationPass());
+
----------------
psamolysov wrote:
> nikic wrote:
> > I don't think we need a late module pass for this, it can be part of the normal module optimization pipeline (post module simplification).
> I'm afraid I didn't get your comment. What I tried is to add `DAE` with `MIWP.addModulePass` but such module passes run before call graph passes.
Oh, I think I have got the idea: instead of adding the `DAE` with `MIWP.addLateModulePass()` just after adding the `ArgumentPromotion` in the `buildInlinerPipeline`, to add `DAE` after this line with usual `addPass`:

```
MPM.addPass(buildInlinerPipeline(Level, Phase));
```

This makes sense when we removing the original point of adding the `DAE` pass, because the `buildInlinerPipeline()` function is called not in every case. Actually, we just moving `DAE` after inlining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128830