[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

nikic added a comment. In D145265#4478621 , @aeubanks wrote: > @nikic could you try running this over the rust tests again? Ignoring some unrelated powerpc data layout failures, these codegen tests fail: [codegen] tests/codegen/issues/issue-45222.rs

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added a comment. @nikic could you try running this over the rust tests again? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 ___ cfe-commits mailing l

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 537871. aeubanks added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: clang/test/Headers/mm_malloc.c llvm/lib/Passes/PassBuilderPipelin

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

nikic added a comment. Here are two inputs that currently fold down to constants and no longer do so with this patch: https://gist.github.com/nikic/4a714ea550bf2252543570585f642af2 These need further reduction for PhaseOrdering tests, but should be a good starting point for analysis... Reposi

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

nikic added a comment. Unfortunately I'm seeing a number of Rust optimization regressions with this change. I'll try to reduce those to some PhaseOrdering tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D1

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

asbirlea accepted this revision. asbirlea added a comment. This revision is now accepted and ready to land. This went through a few rounds of testing on our internal benchmarks and it's at a point where there are no meaningful run-time regressions observed, but the compile-time improvements rema

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added inline comments. Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:476 + FPM.addPass(EarlyCSEPass()); + nikic wrote: > Why the extra EarlyCSE pass in the `O1` pipeline? forgot to update the commit message, see that Comment

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 523182. aeubanks edited the summary of this revision. aeubanks added a comment. update commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: clang/

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

nikic added inline comments. Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:476 + FPM.addPass(EarlyCSEPass()); + Why the extra EarlyCSE pass in the `O1` pipeline? Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:547 + Simplify

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 523175. aeubanks added a comment. rebase keep the existing EarlyCSE/InstCombine ordering Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: clang/test/Headers

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added a comment. this regresses clang/test/Headers/mm_malloc.c (added here ) because we now don't run instcombine before the inliner, and we lose return value alignment info when inlining. perhaps this is still a tradeoff worth making Repository: r

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added a comment. In D145265#4225332 , @asbirlea wrote: > Nit: the diffs would be easier to analyze with "-fno-discard-value-names". done, see commit description the IR diffs look pretty good now, I'm not seeing any major regressions Repositor

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 513731. aeubanks edited the summary of this revision. aeubanks added a comment. fix clang test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: clang/test/Co

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 513728. aeubanks added a comment. update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: llvm/lib/Passes/PassBuilderPipelines.cpp llvm/test/Feature/optnon

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

asbirlea added a comment. Nit: the diffs would be easier to analyze with "-fno-discard-value-names". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 ___ cfe-com

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added a comment. got IR diffs for llvm-test-suite by dumping the IR in clang after running the optimization pipeline (-O3), IR file name is md5 of the module identifier @@@ -1093,6 -1093,6 +1096,16 @@@ void EmitAssemblyHelper::EmitAssembly(B std::unique_ptr ThinLinkOS, DwoO

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

nikic added a comment. I think this (removing GlobalCleanupPM) makes sense at a high level, but this is a pretty substantial pipeline change and will need more thorough evaluation (e.g. looking at IR diffs on test suite). For example, it is not at all obvious to me that moving InstCombine befor

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks updated this revision to Diff 508167. aeubanks added a comment. add FIXME to phase ordering test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145265/new/ https://reviews.llvm.org/D145265 Files: clang/test/CodeGen/thinlto-distributed-ne

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

aeubanks added inline comments. Comment at: llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll:9 ; CHECK-NEXT: if.end: +; CHECK-NEXT:[[TMP0:%.*]] = add i32 [[A:%.*]], 1 +; CHECK-NEXT:[[CONV:%.*]] = and i32 [[TMP0]], 8 aeubanks wrote: > this is an instcom