Sorry, forgot to add cfe-commits to the original diff.

LGTM.

LGTM too. One day I'm going to change all MOPs to MOp when you're not looking anyway.

Changed my mind again, I won't be a part of this.

I'm back in.

(after Matt's points).

LGTM.

LGTM.

Could you fix up the ARM64 selection code as I mentioned earlier so we don't generate different instructions?

@omjavaid can you look over the lldb changes? I don't have the hardware to be able to actually run this test but I've tried to relax the checks.

In D75555#1906391, @omjavaid wrote:

Hi @aemerson this change caused a regression in lldb testsuite on AArch64/Linux. Apparently stepping gets affected by your change. Kindly revisit.

I have temporarily reverted this change.

http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/2182

Hi, I just tried out this patch locally and I'm seeing failures running the tests:

Failing Tests (3):
    LLVM :: CodeGen/AArch64/machine-outliner-cfi.mir
    LLVM :: CodeGen/AArch64/machine-outliner-noreturn-save-lr.mir
    LLVM :: CodeGen/AArch64/machine-outliner-side-effect.mir

Which part of the langref are you referring to?

LGTM.

Now with test updates.

Actually this is missing a bunch of test updates....

In D75207#1896372, @volkan wrote:

In D75207#1894581, @aemerson wrote:

The change itself is ok but is there any way to have more granular overriding of this behavior, instead of overriding the whole shouldLocalize()?

We can have two functions (one for checking the opcode and another one for the cost), but this doesn't allow targets to localize target specific instruction. That's why I decided move the whole thing to TargetLowering. Do you think having two targets hooks would be better?

In D75086#1894721, @qcolombet wrote:

I understand that cross copies are bad, but what I don't understand is why don't we form an extended load? That would apply nicely to your example.

I see what you mean. Essentially we would take the code size/perf hit with the specific MIR example I used, but instead try harder to form the extending load ops and ensure those are assigned to gpr banks. There might be other issues with G_EXTRACT but we'll see.

The change itself is ok but is there any way to have more granular overriding of this behavior, instead of overriding the whole shouldLocalize()?

I committed before I saw your message.

In D75086#1894223, @qcolombet wrote:

s16 operations, if legal, just get selected to gpr32 regclass instructions. E.g.

%ld:gpr(s16) = G_LOAD ...
=>
%ld:gpr32 = LDR_16 ...

That's interesting, because that means you have inreg extension where you would have regular extension in the IR.
E.g.,

s16 = G_LOAD
s32 = G_ZEXT s16

=>

gpr32 = LDR_16
gpr32 = G_ZEXT gpr32

I would have expected that instead we would have combined loads with the related extension to from an extended load.

Yes, on AArch64 all sub-32bit loads give a free ZEXT. We catch this case in the selector if for some reason we have a redundant ZEXT.

Anyway, I guess this ship has sailed.

LGTM.

In D75086#1893996, @qcolombet wrote:

Thanks for the clarification Amara.

Now a follow-up question:

If this happens, the PHI operand with a GPR RegBank gets the gpr32 regclass assigned to it (we don't have anything smaller to represent types like s16 on GPR on AArch64).

How do you represent s16 values into gpr32?

s16 operations, if legal, just get selected to gpr32 regclass instructions. E.g.

%ld:gpr(s16) = G_LOAD ...
=>
%ld:gpr32 = LDR_16 ...

For things like G_ADD we legalize to s32, but s16 itself as a type isn't illegal.

I would have expected that if you have to widen the size when assigning the actual register class, then legalization should have actually widen the value. Otherwise, that means we have to manually deal with "dangling" bits everywhere else.

Forgot to answer your first question:

I would expect G_PHI to be selected to PHI (i.e., just a opcode change) and then the phi elimination pass will insert the cross copies for you.

Where is this going wrong?

Thanks for taking a look. Yes the G_PHI gets selected to PHI. This is normally ok even in the case where we have operands with different regbanks. However on AArch64 this doesn't work when the type size is less than 32 bits. If this happens, the PHI operand with a GPR RegBank gets the gpr32 regclass assigned to it (we don't have anything smaller to represent types like s16 on GPR on AArch64). Then the FPR bank operand gets the appropriately sized FPR16 regclass assigned. These two registers have different sizes, and copyPhysReg doesn't know how to deal with copies between a gpr32 and an fpr16 or vice versa. I thought about maybe teaching copyPhysReg to deal with that, but the fact that we have to deal with subregisters (because of the differing sizes) makes me nervous and didn't seem like the best place to fix it.

Looks reasonable.

LGTM.

LGTM.

LGTM with nits.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM with nit.

LGTM.

In D73030#1857864, @arsenm wrote:

In D73030#1857808, @aemerson wrote:

If this change is made then CPU targets will need to create the new G_INTRINSIC_(DIVERGENT?) opcodes anyway, since not having sinkable intrinsics (like many vector ops, and some exotic scalar ones) won't be acceptable for optimization purposes. So I think we will need the two extra opcodes anyway. Unless we have a different mechanism to communicate this property, like an MI flag? Actually, the issue we had with "tail" attributes on the memcpy intrinsics might also benefit from finding a better solution to this properties issues.

Eventually, but none of these are problems I want to work on solving right now. My goal is to avoid subtle problems in the future, so I still think we should just go with this for now.

In D73030#1857726, @arsenm wrote:

In D73030#1857708, @aemerson wrote:

I'm not 100% sure on what the convergent attribute means here, but assuming that it means it's not allowed to sink instructions into control flow etc, I don't think we should be marking these generic intrinsic ops with it. On arm64 we can use G_INTRINSIC to represent user-written intrinsic instructions which have no problem with being made control dependent.

The intrinsic opcodes need to conservatively assume it could call an intrinsic with any property. These can be used to call an intrinsic or call site marked as convergent, so they have to be convergent to be conservatively correct. Since nothing in the globalisel pipeline currently does any control flow sinking or anything, it's not worth the hassle of optimizing this by adding an additional 2 G_INTRINSIC* variants. Until that comes up, I'd rather leave this in the safe state so somebody doesn't silently break convergent handling at some point in the future.

The localizer is one pass that can sink instructions, although it currently doesn't do intrinsics yet.

I'm not 100% sure on what the convergent attribute means here, but assuming that it means it's not allowed to sink instructions into control flow etc, I don't think we should be marking these generic intrinsic ops with it. On arm64 we can use G_INTRINSIC to represent user-written intrinsic instructions which have no problem with being made control dependent.

Can we have one extra test checking for a chain of G_XORs?

LGTM.

It's a real struggle for me to review AMDGPU legalizer changes sometimes. I wish we had smaller, more obvious for the change being made.

LGTM.

Didn't mean to reject.

LGTM.

In D73403#1849250, @arsenm wrote:

In D73403#1848263, @paquette wrote:

This is causing an assertion error in consumer-typeset for AArch64 at -O3.

llvm-test-suite/MultiSource/Benchmarks/MiBench/consumer-typeset/z29.c

Assertion failed: (Res.getLLTTy(*getMRI()).isScalar() == Op.getLLTTy(*getMRI()).isScalar()), function buildExtOrTrunc, file /Users/gruyere/llvm-project/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp, line 460.

Any thoughts?

What does the failing GEP look like?

LGTM.

Committed in 610f1d22f149 but with wrong phab link in commit msg.

Could you re-upload your patch so it can be reviewed?

Oops, linked to the wrong review in the commit message and ended up auto-closing this by mistake.

Forgot to rename test file.

I've made the test XFAIL for now. Address other comments.

LGTM.

@arsenm Matt when I run the update_llc_test_checks script on the llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.update.dpp.ll test, it crashes with a backtrace like this:

Assertion failed: (Opcode < NumOpcodes && "Invalid opcode!"), function get, file /Users/aemerson/work/oss/co1/src/llvm/include/llvm/MC/MCInstrInfo.h, line 4
frame #4: 0x00000001000bdef3 llc`llvm::MCInstrInfo::get(this=0x0000000110010248, Opcode=4294967295) const at MCInstrInfo.h:45:5
frame #5: 0x000000010083b8d7 llc`llvm::SIInstrInfo::getMCOpcodeFromPseudo(this=0x0000000110010240, Opcode=3758) const at SIInstrInfo.h:939:12
frame #6: 0x00000001008277e3 llc`llvm::SIInstrInfo::getInstSizeInBytes(this=0x0000000110010240, MI=0x000000010f879b28) const at SIInstrInfo.cpp:6067:29
frame #7: 0x0000000101f9bf44 llc`(anonymous namespace)::BranchRelaxation::computeBlockSize(this=0x000000010f03fb60, MBB=0x000000010f8bbf28) const at BranchRelaxation.cpp:168:18
frame #8: 0x0000000101f9b7be llc`(anonymous namespace)::BranchRelaxation::scanFunction(this=0x000000010f03fb60) at BranchRelaxation.cpp:158:39
frame #9: 0x0000000101f9b264 llc`(anonymous namespace)::BranchRelaxation::runOnMachineFunction(this=0x000000010f03fb60, mf=0x000000010f0566f0) at BranchRelaxation.cpp:555:3

I'm replacing a G_MUL with an equivalent G_SHL here, is there an issue with doing this for AMDGPU?