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

psamolysov added a comment. @aeubanks Thank you very much for the re-landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830 ___ cfe-commits mailing list cfe-c

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

aeubanks added a comment. that was fixed and I've relanded the patch hopefully there aren't any more CGSCC issues this uncovers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830 ___

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

aeubanks added a comment. I've run into https://github.com/llvm/llvm-project/issues/56503 after this patch so I've reverted this for now while I'm fixing that. Sorry for this patch exposing so many pre-existing issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

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

zequanwu added a comment. It's causing chromium test failure. [ RUN ] PartitionAllocPCScanTest.StackScanning ../../base/allocator/partition_allocator/starscan/pcscan_unittest.cc:682: Failure Value of: IsInFreeList(root().ObjectToSlotStart(dangling_reference)) Actual: true Expect

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGb10a341aa5b0: [Pipelines] Introduce DAE after ArgumentPromotion (authored by psamolysov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://

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

psamolysov added a comment. @aeubanks Thank you for the investigation! I believe this patch can be re-landed after D132764 is committed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.l

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

aeubanks added a comment. sent out https://reviews.llvm.org/D132764 to fix the CGSCC crash Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830 ___ cfe-commits maili

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

aeubanks added a comment. reduced: define void @a() { entry: %call = call float @strtof(ptr noundef null, ptr noundef null) ret void } define internal float @strtof(ptr noundef %0, ptr noundef %1) nounwind { entry: ret float 0.0 } `./build/rel/bin/opt -passes='inline,a

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

Michael137 added a comment. Noted, thanks for the heads up AFAIK, the only test that'd now fail on the debug-info side is: https://reviews.llvm.org/D132664 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830

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

psamolysov added a comment. No problem, I've reverted the commit while I need some time to build `clang` with the reverted commit even for make it clear the commit is guilty. I'm sorry. It's very interesting, in @mstorsjo case, a function from the standard `C` library is used: `strtof`. When I

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

dyung added a comment. In D128830#3751368 , @dyung wrote: > We also saw this assert on our Windows build, and it also can be reproduced > in Linux: > > $ cat test2.c > static char *getenv(char *) {} > void foo() { getenv(""); } > $ ~/src/upstream

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

dyung added a comment. We also saw this assert on our Windows build, and it also can be reproduced in Linux: $ cat test2.c static char *getenv(char *) {} void foo() { getenv(""); } $ ~/src/upstream/879f5118fc74657e4a5c4eff6810098e1eed75ac-linux/bin/clang -c -O3 test2.c

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

psamolysov added a comment. @mstorsjo Thank you very much for the information. Unfortunately, our tests didn't catch this problem. I've reproduced this on Windows even w/o mingw. Some time is required for triaging. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

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

mstorsjo added a comment. This triggers failed asserts for me: $ cat repro.c static float strtof(char *, char *) {} void a() { strtof(a, 0); } $ clang -target x86_64-w64-mingw32 -w -c repro.c -O3 clang: ../lib/Analysis/CGSCCPassManager.cpp:958: updateCGAndAnalysisManagerForPass(llvm::

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

psamolysov added a comment. Colleagues, thank you for the discussion. @aprantl @Michael137 Thank you very much for the patch and the test changes and confirmation. I've pushed the patch again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/n

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

Michael137 added a comment. In D128830#3746153 , @aprantl wrote: > I think we can "fix" the test with the following patch: > > diff --git a/lldb/test/API/functionalities/unused-inlined-parameters/main.c > b/lldb/test/API/functionalities/unused-inlined-

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

aprantl added a comment. I think we can "fix" the test with the following patch: diff --git a/lldb/test/API/functionalities/unused-inlined-parameters/main.c b/lldb/test/API/functionalities/unused-inlined-parameters/main.c index f2ef5dcc213d..9b9f95f6c946 100644 --- a/lldb/test/API/function

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

dblaikie added a comment. In D128830#3745031 , @Michael137 wrote: > FYI, this broke the LLDB build bot: > https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46324/execution/node/74/log/ > > Looks like we're testing that inlined unused parameters

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

aeubanks added a comment. I'd try compiling with `clang -g2` to see the effects of this patch on the debug info in the IR Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830 _

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

psamolysov added a comment. I tried to triage a bit. The test `lldb\test\API\functionalities\unused-inlined-parameters\TestUnusedInlinedParameters.py` compiles the code in `main.c` with `-O1` and generates the following IR for the `@f` function: ; Function Attrs: alwaysinline nounwind uwtabl

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

Michael137 added subscribers: aprantl, dblaikie. Michael137 added a comment. In D128830#3745069 , @psamolysov wrote: > @Michael137 Thank you very much for the information! > > I'm not sure, but it looks like the introduced change of the `readnone` > att

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

psamolysov added a comment. @Michael137 Thank you very much for the information! I'm not sure, but it looks like the introduced change of the `readnone` attribute to `readonly` might make impact on DWARF. Unfortunately, I have no idea should this changes in DWARF be fixed or just it is enough t

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

Michael137 added a comment. FYI, this broke the LLDB build bot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46324/execution/node/74/log/ Looks like we're testing that unused parameters optimise out but that's not the case anymore `AssertionError: '(void *) unused1 = ' not found i

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

psamolysov added a comment. @aeubanks Thank you very much for the benchmark results and review. I've landed the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3f20dcbf708c: [Pipelines] Introduce DAE after ArgumentPromotion (authored by psamolysov). Changed prior to commit: https://reviews.llvm.org/D12883

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

aeubanks accepted this revision. aeubanks added a comment. This revision is now accepted and ready to land. sorry for the big delay, I did run the benchmarks and it looks fine ran this through llvm-compile-time-tracker, looks good https://llvm-compile-time-tracker.com/compare.php?from=552b59b9e6

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

psamolysov added a comment. In D128830#3647168 , @aeubanks wrote: > ... but let me run some internal benchmarks on this patch @aeubanks Sorry for the late answer. Did you have a chance to run the benchmarks? If so, could you share the results? Reposit

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

aeubanks added a comment. I think even with the readnone -> readonly change this patch should be fine, but let me run some internal benchmarks on this patch the func-attrs change can come later Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/n

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

psamolysov updated this revision to Diff 443860. psamolysov added a comment. Return the `PostOrderFunctionAttrsPass` pass back on its original place in the pipeline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.or

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

psamolysov added a comment. @aeubanks Hmm, if I correctly get your comment, I should revert this patch to the state before the proposed solution with moving the `PostOrderFunctionAttrsPass` at the end of the `buildInlinerPipeline` function regardless of the `readonly` instead of `readnone` regr

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

aeubanks added a comment. (and we should definitely separate that out into its own patch first) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128830/new/ https://reviews.llvm.org/D128830 ___ cfe-commits

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

aeubanks added a comment. I'd tried moving func-attrs after the function simplification pipeline and perf-wise on internal benchmarks it looks fine, but it causes some compile time regressions: https://llvm-compile-time-tracker.com/compare.php?from=d3dd6e57fe84e90cadcdc78fa71d632f6573f156&to=59

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

psamolysov updated this revision to Diff 443661. psamolysov added a comment. [Pipelines] Fix the Clang :: CodeGenCoroutines/coro-elide.cpp Now, the `%_Z5task1v.Frame` type doesn't contain a field of the `%_Z5task0v.Frame` one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

psamolysov updated this revision to Diff 443631. psamolysov added a comment. @aeubanks Thank you for the great explanation. I've applied your suggestion and re-uploaded the patch. The test `unused-nonnull.ll` has been fixed with keeping the `readnone` attribute. Repository: rG LLVM Github Mo

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

aeubanks added a comment. moving DAE after the function simplification pipeline makes sense In D128830#3622736 , @psamolysov wrote: > In D128830#3622467 , @fhahn wrote: > >> Do we need to retain the run of `Dead

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

psamolysov updated this revision to Diff 441462. psamolysov added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix `Clang :: CodeGen/thinlto-distributed-newpm.ll` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128