I would rather we keep it. Although like you said we could piggyback on addRegBankSelect, it is clearer to have a separate method and that's consistent with the other GISel passes.

LGTM.
One nit below.

Looks reasonable (haven't looked at the ARM part.)
The change in shrink wrapping doesn't look correct to me though. Is that particular change require?

LGTM

Are you planning on fixing the regression in the near future? If so splitting the patches this way seems fine

@arsenm ping^2 on the AMDGPU test changes.

LGTM, thanks!

Thanks for the test case!

For the test case, couldn't you add a isVariadic instruction with different patterns in one of the TableGen test?

Looks sensible to me.

@arsenm ping on the AMDGPU test changes.

Thanks Philip for double checking.

I found a latent bug that has now more chances to trigger with the relaxations that this patch brings.
I've updated the patch to fix the bug, but I have now a bunch of "regressions" on AMDGPU test cases because the fix is too conservative.
Is this still okay to land?

• Fix a latent bug
• Update AMDGPU tests

• Remove ADJUSTxxx operations in the tests

Thanks for the quick review @arsenm!

LGTM.

• Replace some unsigned into Register

This was just a patch to illustrate a possible direction.

• Remove asserts already covered by the verifier
• Put .getType calls into a variable.

• Replace check with an assertion
• Update comments with pre-conditions

• Use ArrayRef instead of const SmallVectorImpl&
• Add tests with pointer types.

LGTM

Hi Matt,

LGTM

LGTM

The swifterror test change in this patch is one example. Live ranges over function calls can be problematic if under register pressure, which can cause additional moves, if not spills. There are others of course

Hi Amara,

I don't think this patch is correct in the sense that having statepoints depend on GreedyRegAlloc is a non-starter.
Indeed, that means that now any configuration that doesn't use greedy reg alloc (like fast or basic reg alloc) will generate incorrect code with respect to state point spilling.

Suggestion of a small refactoring below.

Hi Jinsong,

LGTM with a test case.

LGTM for the generic part.

It was added by D46315

Remove loop entirely. Ultimately determineCalleeSaves adds everything from MCRegAliasIterator anyway

Let's go with that for now with one caveat. Does the code build without ANDGPU?
I.e., does the callingconv enum get generated?

General direction LGTM but there's a bug in the current patch unless I am mistaken.

I second Daniel's comment, this is RegBankSelect's job to do this choice and the Legalizer shouldn't need to know about f64 vs. s64.

Maybe adding an MCRegister that's nearly identical might be a future improvement? That way it could enforce MCRegister->Register is OK, but Register->MCRegister is not

Thanks for doing this Matt, this is a long due clean-up.
I haven't looked carefully at all the changes, but the general direction LGTM.