[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

mtrofin updated this revision to Diff 478339. mtrofin added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. fixed the other 2 scripts Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138836/new/ https://reviews.llvm.o

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

mtrofin updated this revision to Diff 478343. mtrofin added a comment. Herald added a subscriber: pcwang-thead. missed one Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138836/new/ https://reviews.llvm.org/D138836 Files: clang/test/utils/update

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

mtrofin added a comment. In D138836#3954782 , @lebedev.ri wrote: > Thanks! > There's also the one for MCA, but this situation basically never happens for > those tests. Yup, checked that and the mir one. The latter seems to be implementing its own "ad

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

mtrofin added a comment. In D138836#3954851 , @lebedev.ri wrote: > In D138836#3954850 , @mtrofin wrote: > >> In D138836#3954782 , @lebedev.ri >> wrote: >> >>> Thanks! >>

[PATCH] D138836: [UpdateTestChecks] Fix `update_test_checks.py` to add "unused" prefixes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG255e7e1c21ee: [UpdateTestChecks] Fix `update_*_test_checks.py` to add "unused" prefixes (authored by mtrofin). Repository: rG LLVM Github Monorep

[PATCH] D139006: [UpdateTestChecks] Match define for labels

mtrofin accepted this revision. mtrofin added a comment. LGTM, but I'd wait for Johannes to review it, too (because of e67f6477fd1ed29acbeddf8482c25d8db826912f . I for one don't quite follow the reasoning there wrt adding the

[PATCH] D72547: [llvm] Make new pass manager's OptimizationLevel a class

This revision was automatically updated to reflect the committed changes. Closed by commit rG7acfda633f13: [llvm] Make new pass manager's OptimizationLevel a class (authored by mtrofin). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72547/new/ https

[PATCH] D73307: Unique Names for Functions with Internal Linkage

mtrofin added a comment. In D73307#1839829 , @MaskRay wrote: > The code change seems fine, but the test requires some changes. I haven't > followed Propeller development, but hope someone with profile experience can > confirm InternalLinkage is the only

[PATCH] D73307: Unique Names for Functions with Internal Linkage

mtrofin added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp: + !getModule().getSourceFileName().empty()) { +llvm::MD5 Md5; +Md5.update(getModule().getSourceFileName()); Just a thought: md5 is a non-bijective transformation, afa

[PATCH] D73307: Unique Names for Functions with Internal Linkage

mtrofin added a comment. In D73307#1839984 , @MaskRay wrote: > In D73307#1839880 , @mtrofin wrote: > > > In D73307#1839829 , @MaskRay wrote: > > > > > The code change seems f

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

mtrofin added a comment. In D143624#4115986 , @aeubanks wrote: > __clang_hip_math.hip is annoying... > > We'll need to remove the `MandatoryFirst` inliner in > `ModuleInlinerWrapperPass`, although not sure if @mtrofin has any issues with > that or not

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

mtrofin accepted this revision. mtrofin added a comment. This revision is now accepted and ready to land. lgtm. Like @aeubanks was saying, let's just give a bit of time (1 month or so?) between when this lands and until we clean up the "mandatory" notion from the advisor, just to make sure nothi

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

mtrofin added a comment. In D143624#4116546 , @aeubanks wrote: > Had a chat offline with @mtrofin, wanted to be clear for future purposes that > we do need the separate AlwaysInliner pass because it's used in -O0 and > constructing a call graph there is

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

mtrofin added a comment. In D143624#4315508 , @nikic wrote: > In D143624#4315468 , @dmgreen wrote: > >> It looks like there is quite a lot more optimization that happens to the >> function being always-inlined (_

[PATCH] D149800: [WIP][PGO] Add ability to mark cold functions as optsize/minsize/optnone

mtrofin added a comment. high level question, why not have it as a pass that runs after profiles (of whatever kind - instrumented or sample-based) are ingested. The pass would attribute functions as described. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

mtrofin added inline comments. Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:495 + InstrProfKind getProfileKind() const override { +InstrProfKind ProfileKind = InstrProfKind::Unknown; This looks a lot like line 290, can it be refactored (or a

[PATCH] D119876: [nfc][codegen] Move RegisterBank[Info].h under CodeGen

mtrofin added a comment. As this is a follow-up of a refactoring, I assume I can just land it (short of file header comments, there was nothing really this patch did more intelligently) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119876/new/ ht

[PATCH] D119876: [nfc][codegen] Move RegisterBank[Info].h under CodeGen

mtrofin marked an inline comment as done. mtrofin added inline comments. Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:43 #include "llvm/Pass.h" +#include "llvm/Support/ARMTargetParser.h" #include "llvm/Support/CodeGen.h" myhsu wrote: > Hmm...did you use

[PATCH] D119876: [nfc][codegen] Move RegisterBank[Info].h under CodeGen

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. mtrofin marked an inline comment as done. Closed by commit rGcb2160760e67: [nfc][codegen] Move RegisterBank[Info].h under CodeGen (authored by mtrofin). Herald added a

[PATCH] D100917: [NewPM] Only invalidate modified functions' analyses in CGSCC passes

mtrofin added a comment. In D100917#2736702 , @aeubanks wrote: > In D100917#2735651 , @nikic wrote: > >> An unfortunate side-effect of this change is that NewPM uses even more >> memory. tramp3d-v4 is up 20% in m

[PATCH] D94644: [Inliner] Inline alwaysinline calls first

mtrofin added a comment. Would running the function simplification pipeline after the always inline pass address the issue? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94644/new/ https://reviews.llvm.org/D94644 _

[PATCH] D94644: [Inliner] Inline alwaysinline calls first

mtrofin added a comment. In D94644#2497180 , @aeubanks wrote: > An alternative is to run the mandatory inliner in the same CGSCC pipeline as > everything else, but the way InlineAdvisorAnalysis is setup made it hard to > implement I don't remember there

[PATCH] D94644: [Inliner] Inline alwaysinline calls first

mtrofin added a comment. In D94644#2498638 , @aeubanks wrote: > It's not an emergency. > The issue with InlineAdvisorAnalysis is that the ModuleInlineWrapperPass > presets the InliningAdvisorMode of the InlineAdvisorAnalysis. We could force > override it

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

mtrofin added a comment. In D93078#2499666 , @jdoerfert wrote: > I think this caused a lot of problems for the Attributor tests. I get the > "conflict output" warning all the time now :( > > These two `UpdateTestChecks` tests also generate the warning. I

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

mtrofin added a comment. In D93078#245 , @jdoerfert wrote: > In D93078#2499982 , @mtrofin wrote: > >> In D93078#2499666 , @jdoerfert >> wrote: >> >>> I think this caused

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

mtrofin added a comment. In D93078#2500032 , @jdoerfert wrote: > In D93078#246 , @mtrofin wrote: > >> In D93078#245 , @jdoerfert >> wrote: >> >>> In D93078#2499982 <

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

mtrofin added a comment. In D93078#2500114 , @jdoerfert wrote: > In D93078#2500040 , @mtrofin wrote: > >> In D93078#2500032 , @jdoerfert >> wrote: >> >>> In D93078#246 <

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

mtrofin added a comment. In D93078#2500124 , @jdoerfert wrote: > In D93078#2500122 , @mtrofin wrote: > >> In D93078#2500114 , @jdoerfert >> wrote: >> >>> In D93078#2500040 <

[PATCH] D94808: [NewPM][Inliner] Move mandatory inliner inside function simplification pipeline

mtrofin added a comment. I think InlineAdvisor::getAdvice needs to take a bool MandatoryOnly, which Inliner then passes. This allows us to then use the same advisor for both the always case and the policy-driven inliner instances, which allows that advisor to correctly book-keep any module wide

[PATCH] D94825: [NewPM]i[Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

mtrofin created this revision. mtrofin added a reviewer: aeubanks. Herald added subscribers: hiraditya, eraman. mtrofin requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Expanding from D94808

[PATCH] D94825: [NewPM]i[Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

mtrofin updated this revision to Diff 317085. mtrofin added a comment. patched tests other than Other/ ones. We want to rationalize emitting remarks, see also D94334 ; I'd prefer first landing the always inlining refactoring, because that would impact the remark

[PATCH] D94825: [NewPM][Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

mtrofin marked 2 inline comments as done. mtrofin added inline comments. Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:463 +return getAdviceImpl(CB); + bool Advice = CB.getCaller() != CB.getCalledFunction() && +MandatoryInliningKind::Always == -

[PATCH] D94825: [NewPM][Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

mtrofin marked 2 inline comments as done. mtrofin added inline comments. Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:463 +return getAdviceImpl(CB); + bool Advice = CB.getCaller() != CB.getCalledFunction() && +MandatoryInliningKind::Always == -

[PATCH] D94825: [NewPM][Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

mtrofin updated this revision to Diff 317133. mtrofin added a comment. Herald added subscribers: wenlei, steven_wu. updated with all tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94825/new/ https://reviews.llvm.org/D94825 Files: clang/test

[PATCH] D94825: [NewPM][Inliner] Move the 'always inliner' case in the same CGSCC pass as 'regular' inliner

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe8049dc3c8a4: [NewPM][Inliner] Move the 'always inliner' case in the same CGSCC pass as… (authored by mtrofin). Repository: rG LLVM Github Monorep

[PATCH] D110891: [inliner] Mandatory inlining decisions produce remarks

This revision was automatically updated to reflect the committed changes. mtrofin marked an inline comment as done. Closed by commit rG7d541eb4d49a: [inliner] Mandatory inlining decisions produce remarks (authored by mtrofin). Changed prior to commit: https://reviews.llvm.org/D110891?vs=377091&

<    1   2