Pre-emptive LGTM with the minor changes done.

Store alignment, volatile and addrspace info in MMOs.

Sigh, I think we also need to pass through the alignments of the src and dest parameters. SelectionDAG does this during building using MemCpyInst::{getDestAlignment(),getSrcAlignment()} and I think we currently lose that information during translation.

Translate the volatile flag to an immediate instead of a G_CONSTANT.

LGTM.

Add a getIntrinsicID() method to MachineInstr.

Factor out the code to get the intrinsic ID from an MI into a helper in Utils.

Allow non-zero address spaces.

LGTM.

I didn't bother reviewing this as the other patch seemed to supersede it. Can it be abandoned?

Test IR:

Thanks for doing this Matt. This bug was blocking landing the power of 2 load/store legalization patch for arm64.

LGTM.

LGTM.

LGTM.

Marking as accepted.

LGTM with comment fix and minor nit.

Add missing test.

New patch to do the optimization in the CombinerHelper and add it to the prelegalizer combiner for AArch64.

This seemed a little presumptive to do for all targets for me. I can try to move it earlier into a Combiner helper, since I don't want to put optimizations into the IRTranslator unless we can't do it in other places.

@paquette ok for arm64?

In D61289#1563253, @arsenm wrote:

I think it would be useful to have generic UBFE/SBFE instructions, which could take the place of sext_inreg. However, those would allow variable offset/width, so it wouldn't bee quite the same.

Nice cleanup, some minor comments.

Can you add an IRTranslator test and commit that separately from the selector changes.

I don't know AMDGPU but the code changes seem ok now.

This change makes a pretty significant implicit design change that I don't agree with. It doesn't seem right that the legalizer routines should have to know about regbanks or how to propagate them. I think the two passes should remain separate, an opt-in change observer seems like the better approach here.

Address comment and rebase on top of unsigned->Register changes.

I decided to not change the existing AArch64 SelectionDAG patterns to use i32 because there were a few too many places that hard coded it in, in lowering as well as the TableGen.

In D63169#1554622, @MaskRay wrote:

@aemerson Hi, I committed rL364124 to fix the link error on -DBUILD_SHARED_LIBS=on builds, but I am not sure if the dependency should be added. Can you take a look?

In D63551#1557215, @rovka wrote:

Hi Amara,

Is something like this OK for intrinsics: https://reviews.llvm.org/differential/diff/206394/ ? It has the disadvantage that if we have an intrinsic with 2 aggregate args, it will be hard to tell where one of them ends and the next one begins. OTOH, I'm having trouble finding intrinsics with even one aggregate parameter, and in fact just using getOrCreateVReg there doesn't cause check-all to crash (nor the test-suite, nor selfhost at -O0 on AArch64).

Alternatively, we could preserve the old interface here and try to move pack/unpackRegs to utils, where we can use them from both the IRTranslator and CallLowering.

Either way, it would be nice to be able to add a test-case before changing this interface, but like I said I don't really know my way around intrinsics that well. Any suggestions?

Addressed comments.

In D63587#1552523, @dsanders wrote:

In D63587#1552333, @aemerson wrote:

In D63587#1552319, @arsenm wrote:

Should something like D56706 go along with this?

Maybe?

@dsanders Should we have a generic post legalizer combiner pass now?

A generic post legalizer combiner is tricky because targets often want very different things. The plan is to have generic rules that target specific combiners can opt into in a reasonable way (e.g. by groups based on a generalized trait of the target) via the CombinerHelper

Thanks so much for working on these patches Diana! It's tricky business, can you be sure to run test suites thoroughly before committing if you haven't already.

LGTM too if we remove the last packRegs use and delete the IRTranslator pack/unpackRegs() implementations.