r372318 with test case revised following suggestion.

LGTM

Typo

Revise isLifeTimeMarker following review comment.

LGTM

LGTM

simplify the patch

In D65452#1606444, @xbolva00 wrote:

But one can use devtoolset on centos/rhel :)

There is a llvm-dev discusion (but now dead, Google blocked it? @jfb @chandlerc) about bumping gcc versions, LLVM wants to use C++-14.

Anyway, this small change looks fine.

In D65452#1606400, @xbolva00 wrote:

Is it worth to workaround old gcc?

Support for gcc 4.8 will be dropped soon(ish) anyway..

BTW, if I read that correct, even though gcc 5.1 is listed as requirement, but that page also mentioned that gcc 4.8 could compile correctly the current LLVM codebase, https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library. As gcc 4.8.x is widely used in many long-term maintained systems, it would be nice to compile LLVM correctly against it.

In D65452#1606342, @MaskRay wrote:

https://llvm.org/docs/GettingStarted.html#id7

GCC >=5.1.0 C/C++ compiler

The current recommendation is >=5.1.0...

Format the commit message

Add fixme.

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