Sorry for the delay! Rebased after recent LoopInterchange changes.

LGTM, assuming you are happy with the perf numbers.

Rebased after rL330875 went in

Ping. The related patches clang/llvm patches have been merged. Clang now accepts and passes on -save-stats= when doing linking + LTO with the gold plugin.

LGTM. Some of the diagnostics can be improved after D45879 lands, I suppose?

Thanks for combining the 64 bit classes! LGTM

Thanks Eli, LGTM!

I still need to look into a compilation issue with CINT2006/403.gcc.

In D45191#1077281, @efriedma wrote:

I am probably missing something, but maybe we could avoid the overflow by doing the re-writing of the induction variables/exit conditions closer to the original range for the induction variables?

The fundamental problem is that the rerolled loop could have a trip count that doesn't fit into a single register, even if the original loop's trip count does fit (e.g. on a 32-bit target, the original loop has a trip count 2^31, we reroll by a factor of four, and the new loop has a trip count of 2^33). So handling overflow correctly in general requires some extra code in the rerolled loop.

Thank you very much for tackling this :)

rebased

I had a high-level look and left some minor comments inline. With respect to the overflow, the original code seems to have the same problem, so this patch does not make things worse.

IMO this leads to a nice improvement for the assembler diagnostics as shown D45880. I think it would be great if we could also make the recent improvements to usability more visible, so they can get wider usage across backends.

Add check to make sure the blocks sets match. I had to add a function to get a handle for a const blocks sets, what do you think? I would be happy to split this up in 2 patches if necessary.

In D45970#1075854, @efriedma wrote:

LGTM

I'm not really happy with LoopInterchangeTransform::restructureLoops overall; it seems like complicated, fragile code. But I don't see a better alternative.

In D45970#1075792, @efriedma wrote:

Needs testcase.

LoopInterchange did that wrong and this additional assertion catches that. The fix for LoopInterchange is at D45970

Fix brace and remove null checks for getExitBlock()

Thank you very much for having a look Eli and sorry for the delay, I needed to find some time to investigate another failure in the test suite with this change. Together with D45970 and this patch, building the test-suite with LoopInterchange + LTO passes.

LoopInterchange did that wrong and this additional assertion catches that. The fix for LoopInterchange is at D45970

LGTM, extension related to SVE register parsing.

LGTM, with the test simplification. Unfortunately there is no easy way to prevent adding the unnecessary nonnull attribute in first place.

Thanks for all the feedback. Abandoning this change now. rL330334 covers one case where we were missing dependencies. There is at least one other case remaining I think and I will try to post a patch for that soonish.

Thanks, LGTM. Please wait with committing a bit, in case anyone else wants to chime in.

Thank you very much for all the feedback & patience :)

LGTM

I think this is a reasonable code movement to make things slightly more modular. LGTM, but please wait with committing till next week, so other people have a chance to chime in.

This is mostly @dberlin 's patch rebased and with the test case.

Thank you very much for the review and the excellent suggestions. I simplified getStatsFileName, added a doxygen comment and changed the arguments of AddGoldPlugin as suggested

Thanks Dave! This should be helpful for LoopInterchange after D35430. Did you measure any the compile-time impact of this?

Add comment to option in gold-plugin.cpp

Nice thanks! LGTM

In D43865#1072149, @dberlin wrote:

So, just to put this on the review: the current code on this review definitely not the right fix compared to the patch i sent :)
I think we should commit that split/cleanup (not sure if you did it as part of the last patch.
It sounds like you had time to bootstrap/test it. If so, please feel free to commit it

LGTM, mechanical change adding support for SVE LD1 (scalar+scalar) instrucitons

Having a single function to detect irreducible CFGs allows us to test it more widely and in case it turns out to be a bottleneck, I think there are a couple of things we can do to improve the performance. The most prominent issue is probably that we re-do work for inner loops
, when loops are processed from inner to outer loop nests.

LGTM

Thanks! Rebased on ToT without D43865.

@dcaballe added a containsIrreduciableCFG function to include/llvm/Analysis/CFG.h in D40874. I do not have time to take a close look today, but it seems at least for the attached test case, containsIrreduciableCFG does the right thing.

Thanks Diego and thanks for your patience! LGTM, but please wait a bit with committing, in case other people people want to raise any additional comments.

The only problem with those functions is that we cannot reverse a filter_iterator at the moment, as needed for D45379. But I suppose that should be fixable too.

Simplified things, only add methods to skip debug instructions.

Thanks for having a look! After @tejohnson 's feedback, I've updated the llvm patch and added a clang patch that adds support for -save-stats with LTO to the clang driver.

Thank you very much for your suggestions Teresa! I've updated the patch to handle a new StatsFile config option in LTO::run. And added stats-file support to LLVMgold.so and llvm-lto2. I will link the patch that updated AddGoldPlugin.

In D45657#1068834, @vsk wrote:

I like the direction this is going in :).

I think having an API like 'instructions_no_debug' or similar in BasicBlock would make the interface friendlier. For one, it'd signal to readers that skipping debug intrinsics is an established pattern. Also, I think it would be easier to change if, e.g we wanted update all existing users of the API to skip lifetime intrinsics too.

It would be great to know in which case this causes a crash. The pass currently should not rely on the DT, but if it is available, it will update it (it is using auto *DTWP = getAnalysisIfAvailable<DominatorTreeWrapperPass>();)

ping

In D45448#1064926, @dberlin wrote:

oh phabricator email and it's inability to be consistent about putting emails in comments.
Read the thread on llvm-commits and it'll make more sense :)

In D45330#1066015, @efriedma wrote:

I think number of instructions eliminated looks quite promising

That's much better than I expected. Although, I'm not sure how much I would trust that number; IPSCCP runs before CorrelatedValuePropagation, so you might be optimizing sequences which would be optimized later anyway.

In D45330#1058762, @efriedma wrote:

This is interesting.

One of the weaknesses of the SCCP algorithm is that it's not path-sensitive; a value is only "constant" if it has the same value on all possible paths. The use of ssa_copy provides a way around this restriction, to some extent: the result of an ssa_copy can be a constant even if the operand is overdefined. That said, this is adding substantial complexity to IPSCCP, and I'm not sure how much benefit you'll get in practice; the example @test2 isn't really compelling.

Fix some problems, SingleSource, MultiSource, SPEC2006 now pass with this patch and LTO. Still needs some cleanup and a few more tests.

Great, I am looking forward to hearing what you find! @jonpa just landed r329884, which should make it easier to add target-specific MachineScheduler's with customized heuristics; basically all one has to do is to implement a custom tryCandidate function. IMO it would make it easier to experiment with heuristics if we would have a separate scheduler with experimental heuristics for AArch64.

LGTM