Can you post the actual performance results? It's hard to judge whether 0.3% cost across the entire compiler is acceptable without knowing the benefits.

LGTM.

LGTM if no one else has comments.

In D42512#1455303, @jyknight wrote:

This patch is not correct -- the crash is not with varargs functions specifically, llvm also crashes for a function declared to explicitly take a float/double. The Win64 calling convention code assigns values to xmm registers, but it should not when sse is disabled.

Additionally, this patch unnecessarily breaks calling vararg functions with _integer_ arguments, which in particular, the EDK2 project does, with windows ABI functions and sse disabled. So, since this breaks existing working code, I'm going to revert this change now, and then propose a different change to address the crash.

Fair enough, I've reverted this in r360589. The test case seemed cumbersome to reduce and commit just for a threshold change, but I'm not sure what the best approach is here yet beyond a depth limit change.

In D61289#1493275, @rovka wrote:

Thanks for the explanations. I think you have some good points. Overall, it still looks to me like we're complicating things for backend writers and introducing a lot of subtle distinctions to keep in mind. It would be useful to hear what other people think about this.

Hi Nikita,

In D60425#1490600, @phosek wrote:

We're seeing a failing assertion when building Skia for AArch64 which seems to have been introduced by this change, see PR41738 for more details.

Hi Hans,

LGTM.

Yep, in that case might as well remove the entire helper.

In D59971#1467947, @arsenm wrote:

In D59971#1467546, @aemerson wrote:

Reviving this as the overall approach was fine, it seems the alignment of non pow2 types is assumed to be the alignment of the next largest pow-2 type, so we don't need to worry about alignment during the breakdown.

I did however change the legalization method to not use extracts/inserts, but instead use extending loads and truncating stores, so that the artifacts get combined away and it Just Works.

I don't like how we do everything in bits, and then the mem operand forces bytes. Would it cost anything to switch MemOperands to also be in bits?

Use MF.getMachineMemOperand()

In D59971#1467947, @arsenm wrote:

In D59971#1467546, @aemerson wrote:

Reviving this as the overall approach was fine, it seems the alignment of non pow2 types is assumed to be the alignment of the next largest pow-2 type, so we don't need to worry about alignment during the breakdown.

I did however change the legalization method to not use extracts/inserts, but instead use extending loads and truncating stores, so that the artifacts get combined away and it Just Works.

I don't like how we do everything in bits, and then the mem operand forces bytes. Would it cost anything to switch MemOperands to also be in bits?

New and improved patch.

Reviving this as the overall approach was fine, it seems the alignment of non pow2 types is assumed to be the alignment of the next largest pow-2 type, so we don't need to worry about alignment during the breakdown.

Yep, forgot to git add it. It's in this new diff, but I'll add it as a separate commit. Ok to proceed?

New patch uses a new CSEConfigBase class as the interface between TargetPassConfig and GlobalISel targets, since CodeGen can't depend on the GlobalISel lib.

Fixed the places where custom legalization was not using full CSE.

Updated patch to not use the custom legalization for non-p0 vectors.

LGTM.