In D60459#1460563, @ronlieb wrote:

The current problem i am trying to resolve in somewhat analogous to hoisting 1/2 of the 64 bit add instruction pair. Although in this particular situation we are actually sinking 1/2 of the instruction pair into a later position within the same block. And yes, i can see how in the future a new machine instruction pass might choose to hoist one of the instructions into a pred BB. I realize i can write additional code to scan a previous block. However i think its better that passes not hoist part of an instruction pair, especially ones such as these. To that end i would rather see my patch assert so that we are forced to deal with such a situation should it arise.
Your example, btw, is a good one for why we should have an IR test for the current problem, rather than an MIR test. An MIR test that runs just before SILoadStoreOptimizer will not detect the affects of a new pass. Whereas the IR test attached to this patch stands a better chance of detecting the issue.

In D59626#1460173, @rampitec wrote:

In D59626#1460006, @bjope wrote:

For the record, I think the "multiple connected components" problem could have been solved by something like this after the call to "createAndComputeVirtRegInterval"

LiveInterval &LI = LIS->getInterval(Reg);
assert(Reg == LI.reg && "Invalid reg to interval mapping");
if (TargetRegisterInfo::isVirtualRegister(LI.reg)) {
  // Check whether or not LI is composed by multiple connected
  // components and if that is the case, fix that.
  SmallVector<LiveInterval*, 8> SplitLIs;
  LIS->splitSeparateComponents(LI, SplitLIs);
}

However, that still leaves the questions about if the pass should do less optimizations related to physregs when LIS is available, or how to preserve LIS if optimizing on physregs.
And I'm not sure about the cost of using splitSeparateComponents compared to a full recompute.

For the record, this code works if applied after createAndComputeVirtRegInterval() call.

In D59626#1460006, @bjope wrote:

For the record, I think the "multiple connected components" problem could have been solved by something like this after the call to "createAndComputeVirtRegInterval"

LiveInterval &LI = LIS->getInterval(Reg);
assert(Reg == LI.reg && "Invalid reg to interval mapping");
if (TargetRegisterInfo::isVirtualRegister(LI.reg)) {
  // Check whether or not LI is composed by multiple connected
  // components and if that is the case, fix that.
  SmallVector<LiveInterval*, 8> SplitLIs;
  LIS->splitSeparateComponents(LI, SplitLIs);
}

However, that still leaves the questions about if the pass should do less optimizations related to physregs when LIS is available, or how to preserve LIS if optimizing on physregs.
And I'm not sure about the cost of using splitSeparateComponents compared to a full recompute.

In D60459#1459784, @ronlieb wrote:

correction: the original input test did NOT have the instructions separated by more than 1 or 2 instructions The resultant output showed the large separation.
The default neighborhood of 10 is probably more than enough.

In D59626#1460006, @bjope wrote:

For the record, I think the "multiple connected components" problem could have been solved by something like this after the call to "createAndComputeVirtRegInterval"

LiveInterval &LI = LIS->getInterval(Reg);
assert(Reg == LI.reg && "Invalid reg to interval mapping");
if (TargetRegisterInfo::isVirtualRegister(LI.reg)) {
  // Check whether or not LI is composed by multiple connected
  // components and if that is the case, fix that.
  SmallVector<LiveInterval*, 8> SplitLIs;
  LIS->splitSeparateComponents(LI, SplitLIs);
}

However, that still leaves the questions about if the pass should do less optimizations related to physregs when LIS is available, or how to preserve LIS if optimizing on physregs.
And I'm not sure about the cost of using splitSeparateComponents compared to a full recompute.

Added regression test.

In D59626#1459926, @rampitec wrote:

In D59626#1459918, @bjope wrote:

In D59626#1458773, @rampitec wrote:

@bjope could you please check if D60419 solves your problem with physreg use removal?

Isn't it weird that this pass now is doing less optimizations when an analysis pass is provided? It might be OK for your use case, but it seems weird from a more general perspective.

FWIW, I'm not sure exactly why we are running this pass in a lot of places in our OOT target (after ISel, before coalescing, post-coalescing, etc). But we have done it for years (since before I started working with this target), and now some of those runs won't do as much cleanup as they used to do (when a LIS is found). Not sure, but maybe it is exactly these physreg removals we aim for, so I'm not sure that solving the LIS preservation problem by not eliminating certain dead instructions is good enough for us. It will take some time to analyse that, so at the moment it is easier for me to just make a work around patch downstream.

Bjorn, thank you for the testcase. It doesn't seem to be easy to quick fix, while I am going for vacation tomorrow. I will create a revert patch for now and try to sort it out later.

I understood someone might be aiming for dead phys regs, but handling phys without recompute can be much more challenging, so probably a full recompute is not unreasonable under such scenario too.

FYI, changes in AMDGPU tests are just because of the liveness rebuild which results in a slightly different and non important allocation change.

In D59626#1459918, @bjope wrote:

In D59626#1458773, @rampitec wrote:

@bjope could you please check if D60419 solves your problem with physreg use removal?

Isn't it weird that this pass now is doing less optimizations when an analysis pass is provided? It might be OK for your use case, but it seems weird from a more general perspective.

FWIW, I'm not sure exactly why we are running this pass in a lot of places in our OOT target (after ISel, before coalescing, post-coalescing, etc). But we have done it for years (since before I started working with this target), and now some of those runs won't do as much cleanup as they used to do (when a LIS is found). Not sure, but maybe it is exactly these physreg removals we aim for, so I'm not sure that solving the LIS preservation problem by not eliminating certain dead instructions is good enough for us. It will take some time to analyse that, so at the moment it is easier for me to just make a work around patch downstream.

In D59626#1458773, @rampitec wrote:

@bjope could you please check if D60419 solves your problem with physreg use removal?

@bjope could you please check if D60419 solves your problem with physreg use removal?

In D59626#1458686, @bjope wrote:

In D59626#1458594, @rampitec wrote:

• I get MachineVerifier problems because the LIS update does not update live ranges for physical registers. Consider an instruction like this dead %42:regs = COPY $a1, if that is removed and it is the last use of the physical register $a1 then we need to shrink the live range for $a1 , otherwise the MachineVerifier will complain about "Live segment doesn't end at a valid instruction".

did you enable MachineDCE in RA in some other target? So far it is only enabled in the AMDGPU, so I am curious where do you see these problems?

Changed option description.

Addressed review comments.

Also renamed decoder namespaces.

Moving pass insertion from common TargetPassConfig into AMDGPU, since most targets do not need it.
Changes to maintain liveness if available remain in the MachineDCE pass, but are a no-op for any other target until they decide to use it late in the RA.

In D59626#1454322, @qcolombet wrote:

Hi @rampitec,

Are the dead instructions marked during the detect dead lanes pass or during the rename independent SubReg pass?

I would expect the former and in that case, it may be easier to just do the code deletion there.

Cheers,
-Quentin

Skip this invocation for targets which do not have subregister liveness.

In D59626#1453765, @craig.topper wrote:

I don't think X86 needs this pass. Everything here looks like bad test construction or the issue in speculative load hardening.

LGTM

Rebased.

LGTM

In D59626#1449373, @rampitec wrote:

Ping.

Since the primary goal of the DCE here is to clean dead subregs I can bail this pass invocation if subreg liveness if not supported by target. What do you think?

Since the primary goal of the DCE here is to clean dead subregs I can bail this pass invocation if subreg liveness if not supported by target. What do you think?

LGTM

LGTM

LGTM

What if we call a real barrier in the same situation?

LGTM

LGTM

Rebased.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

One question though, did it pass precheckin? Make sure it did before submission.

LGTM

LGTM

In D59717#1440117, @arsenm wrote:

This isn't correct, this does impact codegen. This changes whether d16 instruction can be used

LGTM

In D59626#1438258, @arsenm wrote:

In D59626#1438255, @arsenm wrote:

I feel like we just keep spamming DCE runs to solve issues. Is there some reason it's difficult for RenameIndependentSubregs to clean up after itself?

Also, I think making more passes to have to care about LiveIntervals should generally be avoided

Use use_nodbg_instructions().

LGTM

LGTM

LGTM

LGTM