Where do these patterns come from?
I would expect they would be eliminated by InstCombine in the IR.

LGTM

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.

Something just stroke me, why don't you use the FPR bank for all 16-bit values then?

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

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

Thanks for the clarification Amara.

Hi Amara,

• Handle PhysReg and Non-SSA code in general

Here are a few examples of the dump we get:

• No Depth limit:

(lldb) p MI.dumpr(MRI, -1)
%1:_(<4 x s16>) = G_BUILD_VECTOR %6:_(s16), %7:_(s16), %8:_(s16), %9:_(s16)
  %6:_(s16) = G_FPTRUNC %17:_(s32)
    %17:_(s32) = COPY $s0
  %7:_(s16) = G_FPTRUNC %15:_(s32)
    %15:_(s32) = COPY $s0
  %8:_(s16) = G_FPTRUNC %13:_(s32)
    %13:_(s32) = COPY $s0
  %9:_(s16) = G_FPTRUNC %11:_(s32)
    %11:_(s32) = COPY $s0

• Use UINT_MAX instead of Optional to ease the use of the method in the debugger
• Use the dedicated formatted_raw_ostream to pad column

Does that match your view of our conversation?

To https://github.com/llvm/llvm-project.git

9708279c725..618dec2aeff  master -> master

I've been concerned about the register live-ins too (I'm less concerned about the successors issue). Is there documentation on the original decision to disallow physical register live-ins for MBBs before register allocation? We could then check to see if we're violating the original reasoning.

• Use SmallDenseMap instead of DenseMap

I think the argument that because it's possible to do the wrong thing if you try hard enough that you have to protect against that is rather flawed and inconsistent with the existing code in LLVM.

That's true (although you'd need a third vreg with no defs/uses to do it) but as you say there's no reason to do so.

My long term plan for the intra/inter-pass cache is based about caching based on the register too.

Switch the caching to register instead of instruction.

LGTM with a nit. Use a reference instead of a pointer (or check the pointer and fallback to the old addNewBlock when the pointer is nullptr)

and having the builder silently ignore the explicitly setInsertPt. I think that behavior should be opt-in only to CSE, and obvious when it's being done.

Thanks @arsenm for the review!

• Fix typo in comment
• Don't increase depth on COPYs
• Use Register instead of unsigned in added unitests

I do think copies should be ignored for depth. Other places generally try to treat intervening copies as irrelevant

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