LGTM

LGTM

Updated diff LGTM

I pretty much only have comment nits at this point, so I think this LGTM.

LGTM

In the future, can you do a full-context diff? It lets reviewers see the full file.

Remove assert because I forgot that there are a million different generic load instruction variants

Pass the opcode table as a parameter to a helper function

In D87157#2257366, @qcolombet wrote:

Hi Jessica,

This feels to me that we are adding another level of heuristics that could well degrade the performances on other cases. The fact that this runs only on functions with only one basic block and with less than 25 instructions tells me this is really a workaround against a more general problem.
Put differently, I don't see that this is generally useful to be worth the compile time.

LGTM now that I realized I just misread the code here

For AMDGPU this would need a regbank legality check for post-regbankselect combines but I'm not sure what to do about that

LGTM

Oh wait the tests already test what I was concerned about. LGTM with minor nits.

Is there any way to test the hash itself on certain instructions?

Personally I can't see anything wrong with this at this point.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM, assuming the pre-merge check thing is just being wonky and m_GXor exists.

LGTM

LGTM

Shouldn't running the copy_prop combine post-legalization do the same thing as this?

Oh whoops, sorry! Landing now.

In D86228#2226562, @arsenm wrote:

The change itself is fine and more consistent, but this is just making an existing problem worse. Instead of treating signext/zeroext as optimization hints on arguments and call results, we're actually introducing code to do the extension which we were promised was already done. We really should be emitting the equivalent of ISD::AssertZext/AssertSext

LGTM

LGTM

LGTM with a couple nits on comments

I applied D85201 and the tests passed without any conflicts/updates, so it seems fine.

• Add back tests
• Use = default for default constructors
• Add some asserts to applyBuildInstructionSteps

• Use a more functional style + generalize a bit more so this can be used for other combines
• Allow vectors + update test
• Add a helper function for legality checks

LGTM

Forgot to type "differential revision" in commit, closing.

LGTM

LGTM

In D85203#2200550, @arsenm wrote:

In D85203#2200542, @paquette wrote:

Could we have more than 2 possible choices for opcodes? Would it make sense to make this take an arbitrary number of alternatives?

Maybe something like

/// - InsnID - Instruction ID
/// - NumChoices - Number of possible opcodes
/// - Opcode1, Opcode2, Opcode3, ... OpcodeNumChoices - Possible opcodes, ordered by preference
GIM_CheckOpcodeWithAlternatives

I can't think of anything off the top of my head, but it could be useful I suppose.

I considered doing it this way, but I couldn't come up with a case where we would need more than 2. I'd rather use the simpler thing unless there's a concrete need

Also yeah, it looks like this is handled in SelectionDAG::GetDemandedBits:

I didn't port this directly from the DAGCombiner, I originally wrote it just to handle the G_ICMP + G_AND pattern, which appears fairly often in AArch64.

Could we have more than 2 possible choices for opcodes? Would it make sense to make this take an arbitrary number of alternatives?

LGTM

LGTM

This has been sitting for a while, and I can't see anything wrong with it. I think @arsenm's justification makes sense, so I'm just going to go ahead and give this a LGTM.