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

aemerson added a comment. I have another attempt at fixing this in D149918 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143624/new/ https://reviews.llvm.org/D143624 _

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

nikic added a comment. In D143624#4316357 , @aeubanks wrote: > In D143624#4315508 , @nikic wrote: > >> In D143624#4315468 , @dmgreen >> wrote: >> >>> It looks like there

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

aeubanks 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] 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] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

nikic added a comment. In D143624#4315468 , @dmgreen wrote: > It looks like there is quite a lot more optimization that happens to the > function being always-inlined (__SSAT) before this change. Through multiple > rounds of instcombine, almost to the e

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

dmgreen added a comment. It looks like there is quite a lot more optimization that happens to the function being always-inlined (__SSAT) before this change. Through multiple rounds of instcombine, almost to the end of the pass pipeline. The new version runs a lot less before inlining, only runn

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

aemerson added a comment. In D143624#4312905 , @dmgreen wrote: > Hello. It sounds like it is really close to being OK. The combine of the > shift just seem to make things more difficult. > > The `icmp ult i1 %cmp4.i, true` is just a not, would it help if

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

dmgreen added subscribers: nikic, spatel, efriedma. dmgreen added a comment. Hello. It sounds like it is really close to being OK. The combine of the shift just seem to make things more difficult. The `icmp ult i1 %cmp4.i, true` is just a not, would it help if it was actually an xor? Or if the

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

aemerson added a comment. Herald added a subscriber: jplehr. @dmgreen I've been looking at this test again trying to see what's missing. The problem now is that only a VF of 4 is chosen. In the good case, instcombine/simplifyCFG runs so that it simplifies down to an `smin` intrinsic. After this

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

dmgreen added a comment. > I’ll take a look, but this indicates to me that there’s something missing > from the vectoriser or later passes rather than a problem with the inliners > behaviour. Sure. I'm not saying that this patch is wrong. I'm just saying that unfortunately it leads to some pre

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

aemerson added a comment. In D143624#4118341 , @dmgreen wrote: >> It’s not clear from the original commit message why the test is related to >> inlining order? It seems entirely testing vectorization cost model which >> should be insensitive to these ki

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

dmgreen added a comment. > It’s not clear from the original commit message why the test is related to > inlining order? It seems entirely testing vectorization cost model which > should be insensitive to these kind of changes, right? It's a phase ordering test - it's testing the entire pipeline

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

aemerson added a comment. In D143624#4118257 , @dmgreen wrote: > Hello - I had to revert this because of some large regressions we got from > routines in CMSIS-DSP. > > The llvm/test/Transforms/PhaseOrdering/ARM/arm_mult_q15.ll test shows the > problem

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

dmgreen added a comment. Hello - I had to revert this because of some large regressions we got from routines in CMSIS-DSP. The llvm/test/Transforms/PhaseOrdering/ARM/arm_mult_q15.ll test shows the problem - that's why that test exists to ensure that any pipeline changes don't negatively affect

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

aemerson added inline comments. Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1082 + MPM.addPass(AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/false)); + aeubanks wrote: > aeubanks wrote: > > I think we want to insert lifetime intrinsics when optimizin

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

aeubanks added a comment. (my latest comments didn't get addressed in the land) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143624/new/ https://reviews.llvm.org/D143624 ___ cfe-commits mailing list cfe

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

aeubanks added inline comments. Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1082 + MPM.addPass(AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/false)); + aeubanks wrote: > I think we want to insert lifetime intrinsics when optimizing this will never be

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

aemerson 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 i

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

aeubanks added a comment. 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 non-trivial in terms of compile time. Originally the mandatory mode of the normal in

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

aeubanks added a comment. In D143624#4116318 , @aemerson wrote: > Sure, the exponential compile time case is actually just a side benefit here. > The motivating reason for this change is actually to improve code size when > building codebases that make

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

aeubanks added a comment. In D143624#4116171 , @mtrofin wrote: > In D143624#4115986 , @aeubanks > wrote: > >> __clang_hip_math.hip is annoying... >> >> We'll need to remove the `MandatoryFirst` inliner in >> `Mo

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

aemerson added a comment. In D143624#4116114 , @aeubanks wrote: > In D143624#4116080 , @aemerson > wrote: > >> In D143624#4115986 , @aeubanks >> wrote: >> >>> __clang_hi

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

aeubanks added a comment. In D143624#4116080 , @aemerson wrote: > In D143624#4115986 , @aeubanks > wrote: > >> __clang_hip_math.hip is annoying... >> >> We'll need to remove the `MandatoryFirst` inliner in >> `M

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

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

aeubanks added a comment. __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 This isn't quite what I had initially thought, but this might be better. (I was thinking