Could you please update the summary before pushing this?

Update:

• Use comma separated list instead of group naming.

Hi Volkan,

Looks good!

The source change looks good to me, but I would like a bit more work on the test itself.

Looks reasonable to me.

Are the dead instructions marked during the detect dead lanes pass or during the rename independent SubReg pass?

The generic changes look sensible to me.
I would just suggest to add a comment on what VRM is for on the modified methods.

Fix seg fault with PHIs

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Hi Petr,

Landed in r356116.

Hi Matt,

Prototyped a retry legalization process in D59339.
The problem is that this patch is still needed to access the constant, because the legalization artifacts combiner inserts copies instead of updating the users.

For operands that truly need to be constants that shouldn't be legalized, I don't think G_CONSTANT should be used. This is part of the motivation behind D58232 https://reviews.llvm.org/D58232. In SelectionDAG there is the TargetConstant vs. Constant distinction, but I think that makes less sense here, so weird constants should just never be materialized.

Disclaimer: this is just my opinion and in particular I haven't talked with anybody about constants being place holders. So just see my comment as what they are: an individual throwing ideas :).

Update a mips test that I forgot.

Why wouldn't G_CONSTANT be legalized? This doesn't make any sense to me

• Introduce a different variant of getConstantVRegVal instead of overriding the existing one (after talking to @aditya_nandakumar we decided we may want to have more control on what happens when we call this function).
• Add a few comments.

Looks reasonable to me but I'd like @volkan and @aditya_nandakumar to see how it impacts us.

I'm going to hold on that one for a little longer.
I wonder how SDAG handle constant during legalization, because I was not expecting that we would not issue the constant for the new type period.
I suspect it works because constants are just not materialized via instructions and maybe we should just ignore them during legalization. (If a constant is illegal, at the end of the legalization process, it should have any users, right?)

Hi Matt,

LGTM.

Looks reasonable to me.

Nice catch!

The LiveIntervals for the relevant regunits needs to be removed.

We should really just compute this property at some point (same for novregs), but that's for another day :).
LGTM.

LGTM. Nitpick below.

LGTM, one last nitpick below.

Nice catch!

The generic part looks sensible to me.

LGTM, one nitpick below.

Hi Mark,

Hi Mark,

LGTM with nitpick.

Hi Matt,

Hi Reid,

Thanks for the link @jfb.

Update:

• Switch to listing valid ordering instead of invalid ones
• Don't apply the transformation on volatile atomicrmw

Though it's worth pointing out that none of those currently build a description of what it wants to do like the legalizer currently does, they just make the change straightaway when the rules match. Removing in LegalizationAction in favour of just making the change brings the legalizer closer to the others.

The legalizer is already done the way I'm talking about on the rule matching side and performance was marginally better on average than the old setAction() interface.

Should we revert this change to have the conversation going or am I the only one unhappy with the absence of guarantees for the other operands?

Personally, I'd like to replace the LegalizationAction with a function pointer that just performs the desired change and have a standard library of actions that can be called for the common changes.

Thanks for your patience Matt!