Graphics does use region, so we can't break that

Needs some pure MIR tests too (also including negative tests)

This is also further confused by the two types of implicit operands. There are implicit physical register uses as present in the instruction definition, but also implicit uses that code can arbitrarily append to an instruction

I don't think a helper that's only useful in the middle of a partial instruction modification is useful to generally expose

In D99833#2686464, @arsenm wrote:

In D99833#2686416, @paquette wrote:

When I build llc with this patch, with AMDGPU as a target, I get a lot of warnings.

E.g.

Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_128' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^
Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_160' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^

Is that expected?

AV_* classes are for operands that can be one of two different regbanks. You can't unambiguously go back from the class to the register bank. At the moment these are unallocatable classes, although this will probably be changing soon. I'm not really sure what this code should do in this case.

In D99833#2686416, @paquette wrote:

When I build llc with this patch, with AMDGPU as a target, I get a lot of warnings.

E.g.

Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_128' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^
Included from /Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPU.td:1439:
/Users/gruyere/llvm-project/llvm/lib/Target/AMDGPU/AMDGPURegisterBanks.td:13:1: warning: Register class 'AV_160' is already mapped to register bank 'AGPR'.
def VGPRRegBank : RegisterBank<"VGPR",
^

Is that expected?

This is why we should work to migrate to the reverse scavenger which does not depend on kill flags

In D99469#2657766, @foad wrote:

The code change looks fine to me. The added tests aren't actually testing any behaviour that has changed, are they? Is there any reasonable way to add tests for the unable-to-legalize case?

In D100236#2680833, @rampitec wrote:

We should not produce this for gfx803.

LGTM, although should look into updating the MFI serialization

In D98884#2678593, @madhur13490 wrote:

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.

Debugging:

1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
3. Running valgrind also did not reveal any new things.
4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.

However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.

Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.

If it doesn't make an observable difference, there's no real reason to add the extra check?

Can you add a test for the DBG_VALUE case?

Can we just drop the Is from the bit name?

I guess it would be nicer

In D99865#2675219, @thopre wrote:

In D99865#2675176, @arsenm wrote:

It's really hard to write checks for R600, but it's in maintenance mode. The comment doesn't really make sense to me. I doubt this check was ever really correct or stable. You can't really write this test stably and expect specific things in T registers at a specific time. I guess just go with this is it passes?

Unfortunately the test case as updated by this diff FAILs, as can be seen above. Unfortunately the current testcase only succeeds due to a bug in FileCheck which I have a patch for. FileCheck currently count an undefined variable as a match failure which makes a CHECK-NOT with an undefined variable succeed. Does the comment make sense with the old CHECKS (see https://reviews.llvm.org/rG880a80ad07b6ea7ead3a842fc03c74c2247c9486)?

Should we remove those checks altogether?

It's really hard to write checks for R600, but it's in maintenance mode. The comment doesn't really make sense to me. I doubt this check was ever really correct or stable. You can't really write this test stably and expect specific things in T registers at a specific time. I guess just go with this is it passes?

Message / name is somewhat confusing since I assumed this meant the scc register

In D100063#2675053, @rampitec wrote:

If you will run rewriter once no changes are needed at all. Just run the pass right before the rewriter. Although currently you are running it twice. What are your plans about this?

In D100063#2674987, @rampitec wrote:

In D100063#2674983, @arsenm wrote:

Can't this just skip virtual registers? Does it really need to know which mode its in?

All registers are virtual here. It will skip a register if there is no assignment in VRM (and obviously skip physregs).
This is slower as it is though. I can add the check "Reg.isPhysical() || !VRM->isAssignedReg(Reg)" earlier to mitigate it.
Will that help?

Can't this just skip virtual registers? Does it really need to know which mode its in?

Oh whoops, I forgot the hardware totally ignores the modifiers with IEEE mode on (not that this behavior makes any sense). We can't actually do this

LGTM, although we have a few too many constant lookup functions at this point

Typo tyeps in commit message

Probably should have some unit tests for this

Description is misleading. This should be allow a different type for the offset/width operands, which do not necessarily need to be constant

Missing test

If the fallback is enabled, this pass will still run. You need to have the pass check if isel failed and skip the function if it suceeded

In D98515#2660982, @mbrkusanin wrote:

%1:vgpr(s32) = COPY $sgpr0

I wonder if we should ban this in the verifier. It's not wrong, but it sure feels like bad form to allow cross bank copies involving physical registers

Style changes

reverse ping

I guess this is technically correct but I wouldn't spend time getting this accurate

Can this be abandoned now?

I think the block numbering is meaningless and shouldn't be any different between selectors

Is this still needed?

I just realized I've been reading "frame pointer elimination" as "frame index elimination" but this change still doesn't make sense to me. Why do you want to treat these stores as frame instructions when they don't modify the stack pointer?

This seems like a confusion of what frame instructions means. What do these stores have to do with call frames? The stores should reference frame indexes representing fixed frame objects in the incoming arguments or local objects. The frame adjust instructions are for SP modifications in call sequences where instructions are directly referencing offsets off of the SP, which do not have corresponding frame indexes

LGTM. I think "lanes do not die in a function" may not be true in the future, but is good enough for now (not sure whether callers or callees should be responsible for wave termination on kill)

LGTM, though ensuring the right mode in the lowering. We probably won't have MIR atomic expansions anytime soon

Should have isCanonicalize utility function

Implicit operands aren't special. I also think the subreg handling is off

I don't think this should special case statepoints. Any regmask with tied operands should behave the same way

Fix being totally wrong

In D99587#2658501, @sebastian-ne wrote:

We already use this connection in SIInstrInfo::isLegalFLATOffset, there is a check for ST.hasFlatSegmentOffsetBug() && AddrSpace == AMDGPUAS::FLAT_ADDRESS. I guess this is already broken then.

Yeah, that's wrong

In D99507#2658404, @hliao wrote:

It seems to me that we may need to revise CFG lowering to avoid updating EXEC directly and later revise it based on whether the restoring mask needs reloading or not. Here's the brief thought in my mind:

• Instead of lowering CFG early before RA, lower it after RA. As a byproduct, it also remove the need of "terminator" version of exec mask manipulation instructions.
• When CFG is being lowered, it could update EXEC eagerly if the merge point doesn't need to reload the mask; Otherwise, it just needs to translate as what we currently did.

Any suggestions and comments?