It looks reasonable to me. If no one else objects in a couple of days or so, it LGTM.

Next time, when one patch causes a regression that another patch fixes, please, make the one a parent of the other.

Thank you, @courbet.

In D46356#1126217, @RKSimon wrote:

@evandro Does AArch64SchedExynosM1.td and AArch64SchedThunderX2T99.td look correct now please?

In D46356#1124577, @courbet wrote:

Update ExynosM1.

In D46356#1120407, @courbet wrote:

A review of ExynosM1 /ThunderX2, as I'm not familiar with these CPUs.

Thank you.

Include the solution found in D47349.

There's no need to make these methods virtual and override them in the sub target. Rather, modify the limits when the sub target is initialized, as @eli.friedman suggested.

In D47349#1112803, @efriedma wrote:

MaxStoresPerMemcpy = STI.requiresStrictAlign() ? 4 : 16;?

In D47349#1112719, @efriedma wrote:

I meant that you can change the value of MaxStoresPerMemcpy in AArch64TargetLowering::AArch64TargetLowering, instead of overriding getMaxStoresPerMemcpy.

In D47349#1112584, @efriedma wrote:

The AArch64Subtarget is explicitly passed as a parameter to AArch64TargetLowering::AArch64TargetLowering.

In D47349#1111849, @efriedma wrote:

If I'm following correctly, the problem here is that we don't have paired load/store operations for byte operations, so the threshold is roughly double what it should be. StrictAlign isn't really a good proxy for that; it doesn't have anything to do with whether a particular memcpy will lower to paired operations. Ideally, we should probably come up with some callback to give a bonus to paired operations rather than penalize all operations in StrictAlign mode.

In D45098#1111593, @evandro wrote:

In D45098#1111446, @efriedma wrote:

Those numbers look very different from the ones before. Is r332482 making this less profitable somehow? Or is the change all noise?

I'll have to take a closer look at these CPU2000 results, but in proprietary benchmarks this change is still beneficial.

Update test case to keep it from failing due to this change.

Since @sebpop has just left for a deserved vacation, he asked me to babysit his pending patches.

In D45098#1111446, @efriedma wrote:

Those numbers look very different from the ones before. Is r332482 making this less profitable somehow? Or is the change all noise?

In D46356#1089483, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

Yes. They're not done yet. Will let you know when I get the results.

Any news ?

In D46851#1098587, @efriedma wrote:

i8 isn't considered a legal type by the AArch64ISelLowering, so it will never show up in the input to the instruction selector. Given that, I'm not sure it's really meaningful to say i8 maps to any specific register class.

But I guess it wouldn't do any harm to list i8 as part of FPR8? At least, I can't think of any other effects.

Apropos, why is FPR8 defined as untyped and not i8 below?

In D46655#1096661, @efriedma wrote:

LGTM. (We should also add the i8 patterns at some point, but we can do that as a followup.)

I tired this patch on Exynos and the performance of SPEC CPU2000 was virtually neutral, even if slightly negative overall in the integer score. Perhaps the defaults costs are too optimistic about the relative difference between multiply and add?

In D46356#1087529, @courbet wrote:

@evandro Do you have benchmarks to evaluate the impact of this change ?

In D46356#1086531, @javed.absar wrote:

Does FDIV use both the resources for 8 cycles? Writing [8,8] would imply that.

Ping! 🔔

In D39976#1054542, @gberry wrote:

In D39976#1049844, @evandro wrote:

This change is more generic and flexible than FeatureSlowPaired128. This change controls not only when loads and stores are paired, but also other foldings that this pass performs, including the pre or post indexing of the offset register.

That's not what the code looks like it is doing. isReplacementProfitable() is only being called from mergeUpdateInsn(), which is only called when folding to form base register incrementing load/stores.

In D45098#1053201, @efriedma wrote:

Should we check for hasNEON() here? The generic code doesn't know AArch64 has ldp/stp, so we might want to be a little more aggressive to compensate.

In D39976#1040345, @gberry wrote:

In D39976#1027478, @evandro wrote:

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

I'm more confused now. 'FeatureSlowPaired128' controls whether certain load/store opcodes are combined to form paired load/stores. But this change prevents some load/store opcodes from having their base register increment folded in. The two seem unrelated.

Do you intend to consider AArch64TTIImpl::getMinMaxRdxCost() too?

Ping! 🔔

In D39976#998081, @gberry wrote:

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions.

Updated the test case.

That's a sensible alternative.

The test case has scalar types and it seems more interesting to see the cost rising proportionally with the vector factor.

Looks pretty straightforward to me.

Added a test case.

It LGTM, but I wonder how it affects other major targets, like PPC. It's probably a good idea to give them some time to ponder this change.

It LGTM, but I'd wait a while to give a chance for others to chime up.

In D39976#1027475, @gberry wrote:

In D39976#1017864, @evandro wrote:

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

That seems like a worthwhile goal, but this change doesn't really seem to be accomplishing that. If the sched model is being used by a subtarget-specific heuristic, that seems like just a more roundabout way of achieving the same result for your subtarget. Is there any net effect of this change combined with D40107?

I'm not sure what is gained by not performing this optimization, if I understood the gist of it from the new test case below. For even if the register is killed, the pointer adjustment is folded into the load or store and an instruction is eliminated. What other optimizations are expected to be happen should this patch be applied?

This patch results in some image processing benchmarks to improve by over 1% on big Cortex and Exynos cores.