when we insert that copy, if we still follow the current logic of phi-elim, that COPY is already inserted after its use (that CF_END).

It sounds to me that additional hook is a clear change. Or, we need to change the existing hook from const MachineInstr & to MachineInstr & to allow changing instruction. Any idea?

Please review https://reviews.llvm.org/D64946 following your suggestion.

a similar case to https://reviews.llvm.org/D64768

In D64353#1590037, @arsenm wrote:

No matter what, running UnreachableMachineBlockElim is not a fix for this.

In D64353#1574085, @hliao wrote:

In D64353#1574073, @arsenm wrote:

In D64353#1574005, @hliao wrote:

In D64353#1574004, @arsenm wrote:

In D64353#1573974, @hliao wrote:

In D64353#1573922, @arsenm wrote:

I think you should probably fix SILowerControlFlow

the misplacement of def/use is not the fault of SILowerControlFlow but PHIEliminate. However, even PHIEliminate should not be blamed as cf.end is part of MBB prologue. cf.end should not take any input from PHI.

In O2 compilation, there won't be such issue as, in optimized RA, livevariable is used to eliminate PHI better. LiveVariables depends on unreachable-mbb-elimin, which remove the PHI to cf.end.

This isn't a property the pass should rely on. SILowerControlFlow should defend against unexpected iteration order and unreachable blocks

any use before def is invalid MIR, right?

So the MIR is valid before phi elimination, and then becomes invalid after. Removing unreachable blocks earlier to avoid this happening is a workaround. It seems to me like we need to fix PHI elimination or avoid relying on the prolog instructions

my understanding is that MBB prologue instructions are designed NOT to take PHI as input. Like these pseudo instructions for structurized CFG, by design, they don't take any PHI as input. As an alternative approach (not workaround), we could ensure these instructions not taking PHI as input before PHIElim instead of teaching PHIElim to under them, more or less very target-specific stuff.

The verifier should probably check this. We definitely should not hack around this by deleting unreachable blocks that happens to run into this

In D64145#1587690, @arsenm wrote:

LGTM with nit

revise the test case

In D64768#1586690, @jdoerfert wrote:

LGTM, one request and a proposal to fix it slightly differently.

In D64768#1586555, @hliao wrote:

In D64768#1586435, @jdoerfert wrote:

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

I understand. That is what I tried to describe above. My question was: Should stripAndAccumulateConstantOffsets return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into stripAndAccumulateConstantOffsets. I personally tend towards the latter.

Either way, we have to add a comment to the stripAndAccumulateConstantOffsets method explaining the choice!

In D64768#1586435, @jdoerfert wrote:

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

this fixes crashes found on targets where addrspacecast is used.

In D64596#1581524, @arsenm wrote:

What is this supposed to solve?

skip determining callee saved scalar register as well.

please update the doc on transformation metadata

LGTM

In D64421#1576385, @arsenm wrote:

LGTM, although these should probably be the default implementations anyway

In D64353#1574073, @arsenm wrote:

In D64353#1574005, @hliao wrote:

In D64353#1574004, @arsenm wrote:

In D64353#1573974, @hliao wrote:

In D64353#1573922, @arsenm wrote:

I think you should probably fix SILowerControlFlow

the misplacement of def/use is not the fault of SILowerControlFlow but PHIEliminate. However, even PHIEliminate should not be blamed as cf.end is part of MBB prologue. cf.end should not take any input from PHI.

In O2 compilation, there won't be such issue as, in optimized RA, livevariable is used to eliminate PHI better. LiveVariables depends on unreachable-mbb-elimin, which remove the PHI to cf.end.

This isn't a property the pass should rely on. SILowerControlFlow should defend against unexpected iteration order and unreachable blocks

any use before def is invalid MIR, right?

So the MIR is valid before phi elimination, and then becomes invalid after. Removing unreachable blocks earlier to avoid this happening is a workaround. It seems to me like we need to fix PHI elimination or avoid relying on the prolog instructions

cf.end and related MBB prologue instructions should not take PHIs as input. Before LCSSA is turned on, we won't have that IR. After LCSSA, the only possible PHI input is the one from non-latch loop single exit. That PHI is unnecessary and should be removed early to assure that the later passes could relies on the fact they cf.end and etc. won't take PHI as input.

In D64353#1574004, @arsenm wrote:

In D64353#1573974, @hliao wrote:

In D64353#1573922, @arsenm wrote:

I think you should probably fix SILowerControlFlow

the misplacement of def/use is not the fault of SILowerControlFlow but PHIEliminate. However, even PHIEliminate should not be blamed as cf.end is part of MBB prologue. cf.end should not take any input from PHI.

In O2 compilation, there won't be such issue as, in optimized RA, livevariable is used to eliminate PHI better. LiveVariables depends on unreachable-mbb-elimin, which remove the PHI to cf.end.

This isn't a property the pass should rely on. SILowerControlFlow should defend against unexpected iteration order and unreachable blocks

revise test case

In D64353#1573922, @arsenm wrote:

I think you should probably fix SILowerControlFlow

In D64353#1573858, @arsenm wrote:

I'm still confused. When I try your testcase, I see an earlier iterator crash in SILowerControlFlow

In D64353#1573837, @arsenm wrote:

I don't follow how eliminating unreachable blocks fixes this.

In general expecting code placement with the block "prolog" instructions isn't reliable. I had fixed this in rL357634, except it was reverted

In D64353#1573837, @arsenm wrote:

I don't follow how eliminating unreachable blocks fixes this.

In general expecting code placement with the block "prolog" instructions isn't reliable. I had fixed this in rL357634, except it was reverted

revise commit message.

revise the test function name

In D64144#1569828, @arsenm wrote:

Test? I’m also note sure we get much value by tracking these here. I don’t think they are accurate for shaders

FlatScratchInit is set by checking whether there are any stack objects in the early stage of codegen. As we could load MIR in any stage, even after RA, it may set that flag incorrectly by counting spilling stack slot.

NumUserSGPRs and NumSystemSGPRs need updating during the parsing argument info.

minor style fixing

patch is enhanced to remove 'isReg'