LGTM.

Update according to comments.

In D71127#1774339, @dmgreen wrote:

Looks sensible.

Is it worth un-xfailing the test, now that it no-longer crashes? Even if it's check the poor codegen (maybe with some FIXME's), we can show it improving over time. I would use that update script, but I'm not sure how verbose that test will be with it.

D70599 just adds the test, and abandoning this change.

It looks like there's also some stuff in the ARM target (some custom lowering) that needs fixing. I think what I'll do is open a separate review for adding just the test (XFAILed for now), then work on fixing the stuff in the ARM target.

In D70550#1755470, @craig.topper wrote:

I was going to start working on fixing this correctly next week. Can you wait for that?

LGTM

Reducing the repeated lookups in DepthMap seems reasonable, but the changes to the loop where ReachableMap is updated make it quite a bit more difficult to understand (as the task of updating ReachableMap is smushed together with the task of updating NonPhiReachableMap). Could you do just the first part? Or are there cases where this majorly speeds things up?

Would it also make sense to add FCBRT to ExpandFloatResult and PromoteFloatResult? I don't know if it would be possible to test those though.

Tests look good to me.

This will fix https://bugs.llvm.org/show_bug.cgi?id=25610, so it would be good if we could have a test for that as well (and to close that bug once this is committed).

LGTM, with one nitpick.

LGTM, with a couple of nitpicks.

LGTM.

This is causing incorrect code generation for this piece of IR:

In D62152#1508979, @efriedma wrote:

Please verify my understanding of the following is correct:

1. getTypeUnadjustedAlign() is currently only used to compute calling convention alignment for ARM and AArch64.

Yes, the only places it's called are AArch64ABIInfo::classifyArgumentType and ARMABIInfo::classifyArgumentType.

The title and summary of this patch describe a problem, but they don't describe how this patch fixes that problem (which makes it hard to figure out if this patch is correctly fixing the problem). It should be more like "[GlobalMerge] Do XYZ: We have problem ABC, doing XYZ in GlobalMerge fixes it because ...". Also this patch needs tests.

LGTM.

LGTM.

In D57820#1399586, @john.brawn wrote:

It's a bit hard to know whether to accept this or not, given that you says it's "probably better to avoid LDXR". Do you have any data to say if it is any better or not?

It's a bit hard to know whether to accept this or not, given that you says it's "probably better to avoid LDXR". Do you have any data to say if it is any better or not?

LGTM. Looking at it it does make me wonder if we should handle things that aren't ConstantInt, e.g.

bitcast <2 x i16> <i16 0, i16 0> to i32

is still zero, though I'm guessing things like this actually probably never happen and this commit is fine as it is anyway.

Adjust according to review comments, and also add some more tests.

OK, then this looks good to me.

The idea behind allowing Scalar PRE when the addressing mode isn't legal is for some thing like (adjusted from pre-gep-load.ll)

define void @foo(i32 %stat, i32 %i, double* %p, double* %q) {
entry:
  switch i32 %stat, label %sw.default [
    i32 0, label %sw.bb
    i32 1, label %sw.bb
    i32 2, label %sw.bb2
  ]

It may be worthwhile allowing scalar PRE on GEPs that we know won't be combined into the addressing mode of a load/store, i.e. those where TargetTransformInfo::isLegalAddressingMode returns false.

LGTM.

This looks good, but I think that some of the comments need to be adjusted as they're no longer accurate for what the code is doing.

Added an option to enable control flow hoisting, turned off by default.

Looks OK as a fix for the reported bug, with one minor nitpick. Taking a look at PR39625 it looks like there's an underlying problem where we insert a load of useless placeholders, e.g. for

declare void @otherfn()

In D52827#1306034, @mkazantsev wrote:

This rehoisting stuff has been wary for me before, and now I'm completely hesitant about what is going on there. Why do we end up hoisting to some block that does not dominate its users? Do we have a strong reason of doing so rather than finding the correct hoisting destination?

The thing causing the crash was incorrect handling of instructions that need to be rehoisted. When we have hoisted a phi, then hoisted a use of that phi, then if have to rehoist that use we need to make sure that we rehoist it to _after_ the hoisted phi. We can do this by rehoisting to the immediate dominator instead of just rehoisting everything to the original preheader.

Update according to review comments.

In D52827#1295052, @mkazantsev wrote:

The patch fails the following test:

...

Likely some corner case related to conditional branch to the same block for true and false is not handled. Please make sure that it passes before we can proceed with the patch.

Also, we should be disabling this transformation when optimising for size.

A few general comments:

Use SmallVector for HoistedInstructions, rename getHoistedBlock, add LI and DT verification, other small changes.

Use LoopBlocksRPO to generate the worklist.

In D52827#1283474, @mkazantsev wrote:

I generally like what this patch is doing, but the size of code and its complexity overwhelms me. Is it possible to separate the patch into smaller pieces so that it would be easier to review them in isolation? If yes, I'd appreciate that a lot.

Because the ABI says that the GE bits are undefined on entry / return from public interfaces (i.e. non-static functions) I think that it is possible to just look at the current translation unit and make a decision based on that. I'm not sure if just looking at whether the sel intrinsic is used will work though - the GE bits could be read by __arm_rsr("APSR"), or by inline assembly, and maybe there are other situations that I haven't thought of.

A few errors in comments, but otherwise LGTM so long as you fix those errors before committing.

In D52827#1280717, @probinson wrote:

Does this lose debug information? I do very little with optimization passes so I don't know, but it would be nice to keep it in mind.

Update according to review comments.

Having some way to generate a pipeline string which matches what opt does by default is definitely a good thing. I think that long-term splitting up simplifycfg is probably the best way, and I think I'm going to have to do some of that myself as part of D50723 (as the current implementation causes a lot of problems for bugpoint as the pass list it deduces doesn't do the same thing as -Owhatever), but doing it in this way for now seems fine.