In D94827#2505431, @lebedev.ri wrote:

Wow. So much polarization here.

In D94827#2502435, @kuhar wrote:

Wow, this is fantastic. When I first started working on the domtree updater back in 2017, SimplifyGFG seemed like one of the most difficult passes to handle, and I wasn't sure if we ever get there. Very impressive work, @lebedev.ri!

In D94827#2504239, @mkazantsev wrote:

This is awesome. And scary. This is scarily awesome. :)

Why thank you.
It ended up being rather boringly trivial after all to me.
The hard part was coming up with the gradual roll-out part (the flag)

As for the rest of comments, let me split this up into parts.

1. Should SimplifyCFG know how to preserve {,Post}DominatorTree
2. Should SimplifyCFG make use of DominatorTree
3. Should SimplifyCFGPass require DominatorTree

As for 1., for the life of me i can not take that question seriously.
Let me point out that SimplifyCFG is an *Utility*, it can be called from other passes,
and as is painfully visible from the diff, both the DwarfEHPrepare
and AMDGPUUnifyDivergentExitNodes do that already.

By keeping SimplifyCFG illiterate about DomTrees, we force it's users
to also avoid DomTrees to some extent, be it either to just not preserve them,
or recompute them each time after calling SimplifyCFG.

Is that really what we want to be doing?
So if such a question is seriously being asked,
i'd personally suggest to take a step back and really think about it.

3 - ignoring the cost of domtree preservation, this is basically free.
As far as i can tell from the tests, it doesn't result in any new domtree creations,
we'd always already calculate domtree after the simplifycfg invocation,
now we'll just do that earlier.

2 - well, yeah, that is a question of the patch, isn't it? :)
Not sure why it's being met with such surprisingly-hostile comments.

A test case would be nice of course, but with this kind of worklist order issue (select folded before icmp) it's always tricky.

In D94447#2505374, @fhahn wrote:

I believe compile-time is at least partly a motivation for some limitation here, but not in a way that is exposed by CTMark. I think here we mostly want to guard against the worst case (deeply nested AND/OR). Those kind of cases are not really covered by the tests in CTMark (or other parts in the test-suite), but are surprisingly common in practice (e.g. various code-generators that feed into Clang/LLVM). I think it would be desirable to have a limit on the number of items in the worklist to guard against that.

Rebase over more tests, prevent exponential renaming with reused operands.

I'm somewhat skeptical about this change. The motivation seems a bit weak given the costs involved. The costs are:

In D93789#2503709, @craig.topper wrote:

I’m not against this, but do want to mention the help text is incorrect for x86-64. It lists CPUs that don’t support 64-bit. Using one of those CPUs will trigger a fatal error. A better list is fillValidCPUList from X86TargetParser.cpp in lib/Support. It takes a mode argument. That’s the interface clang uses.

Does the same issue apply to ThinLTO?

LGTM

In D94866#2504011, @aqjune wrote:

Okay, I agree that this is more general.
I was cautious about possible compile time degradation - do you have a number for this?

LGTM

Fix typos in mask test.

Ping!

As the discussion is spread out across multiple threads, do I understand correctly that the current consensus is to introduce the -disable-noundef-analysis flag, and explicitly add it to all the relevant tests (rather than adding it to the substitutions)?

I've reported the polly bug at https://bugs.llvm.org/show_bug.cgi?id=48783.

LGTM

LGTM

LGTM. I'm not particularly familiar with this code, but don't see an issue with exposing this information. Currently MCSubtargetInfo directly incorporates the logic for printing a help text to the error stream, which is very dubious design. It should be possible to obtain the same information for printing a help text manually.

I've fixed the issue in b1c2f1282a237e9bc60f1b0020bc7535ca019739 and reapplied this change. The problem was introduced by D91936 and exposed here.

I posted an alternative patch at D94866. It's potentially more expensive, but I think it will cover more of the cases we need.

I think this is a case where being in select form favors us, and we can handle this through a more general transform, rather than the special case that is needed for and/or. The general transform is something along these lines: https://gist.github.com/nikic/4bad7e7e7ecda8d9c9e9633d86836b4d I've wanted to do this for a while, and this looks like a good excuse to make it happen :)

I tested the original three patches plus this one, and the results look good (or at least in line with expectations): https://llvm-compile-time-tracker.com/compare.php?from=33be50daa9ce1074c3b423a4ab27c70c0722113a&to=972a64b1331d1b3022154297593837234a958d0b&stat=instructions

@rnk Sorry, I got my binaries mixed up again... I can reproduce the crash now.

@rnk Thanks the the reproducer! Unfortunately I'm not seeing the crash. I'm at a1be47b4771467998d7549dcd1b9f9cebdaa9af9 directly before your revert using a Release+Asserts build.

In D94363#2499809, @asbirlea wrote:

Do you have an idea if there is overhead of adding the AAR object to each query (each query info object)? (vs the setAAResults being done once for all the AAs available).

Logic looks fine now, though the GEP handling can be made a bit more lax.

The new direction of the patch looks good to me. And yes, I would prefer it if the intrinsic attribute updates were split off into a separate patch.

LGTM

In D94633#2499465, @jdoerfert wrote:

The logic seems sound, could be improved since this does initialize the fixpoint computation with a pessimistic starting value. One comment below, the rest is OK for merging.

I thought about this, but I don't think that we can use an optimistic fixpoint here without some much more sophisticated analysis. Just naively assuming "willreturn until proven otherwise" will incorrectly mark infinite recursion as willreturn:

; Not willreturn.
define void @willreturn_recursion() {
  tail call void @willreturn_recursion()
  ret void
}

So, I run this through the Attributor: https://opt.godbolt.org/z/TqsK86
Not what we were looking for, so here is the non-trivial version: https://opt.godbolt.org/z/rbeedM

Just to make sure we're on the same page, both of those would be bugs in attributor right? The first one shouldn't be unreachable, and the second one shouldn't be willreturn, at least not without more information than is contained in that IR.

I agree for the first one. Mustprogress is missing for unreachable to correct. The second one is correct, or am I missing something?

Compile-time: https://llvm-compile-time-tracker.com/compare.php?from=a3904cc77f181cff7355357688edfc392a236f5d&to=b5ccbbb5b1d8fc76e2291a860e4cf0a4787b77d4&stat=instructions Seems mostly fine, only outlier is kimwitu++ in NewPM-O3 configuration (k.cc has 1.25% regression).

Over in D91837 the reserveForAndGetAddress() implementation contained a check for TakesParamByValue that seems to be gone in this revision. My first guess is that this is the culprit, and we're now always performing the isReferenceToStorage() check.

The last time I tested this in https://reviews.llvm.org/D91837#2464554, the impact was much smaller. Some change between then and the landed version must have regressed things again. I think the numbers are now similar to what we saw before the "small value" optimization was added.

In D94633#2498743, @jdoerfert wrote:

EDIT: I'm fine with this patch, FWIW.

In D94633#2498642, @nikic wrote:

In D94633#2498420, @jdoerfert wrote:

I still think we should check to enable a light-attributor run in the beginning of the pipeline.

I was hoping to get the willreturn-related bugs fixed in time for LLVM 12, so sticking to the existing infrastructure seems like the safer bet :) It probably makes more sense to enable the attributor after branching rather than before.

Fair. As I said, we can enable it in a restricted, lightweight mode first. Could you remind me which bugs?

Thank you @dmgreen and @asbirlea for testing!

Use all_of(instructions(F)).

In D94633#2498420, @jdoerfert wrote:

I still think we should check to enable a light-attributor run in the beginning of the pipeline.

Add test for infinite recursion and irreducible flow.

Fix clang tests. Don't disable MemCpyOpt at O1 (previous version disabled it by accident).

FYI this change had some pretty significant impact on compile-time (https://llvm-compile-time-tracker.com/compare.php?from=4718ec01669b01373180f4cd1256c6e2dd6f3999&to=f748e92295515ea7b39cd687a718915b559de6ec&stat=instructions) and code size (https://llvm-compile-time-tracker.com/compare.php?from=4718ec01669b01373180f4cd1256c6e2dd6f3999&to=f748e92295515ea7b39cd687a718915b559de6ec&stat=size-text). Is that expected?

With this patch applied, this is the impact on InstCombine after flipping the flag: https://gist.github.com/nikic/052815150762859dddaefde10f25374d We can see that there are still quite a few cases where we regress. However, many can be fixed by stronger impliesPoison handling. E.g. we're currently missing icmp X, Y && icmp X, Y and icmp (X & C), C2 && icmp (X, C'), C2'.

LGTM as well.

Yes, I think this is fine to land as-is. Note that SDAG always works on a single basic block, so I don't think there is any value in having "branch on undef is UB" semantics, as we can't infer information from branch conditions anyway.

The thought that the current limitations are compile-time related crossed my mind, but that doesn't seem to be a concern: https://llvm-compile-time-tracker.com/compare.php?from=00f773cf424699d8eb31591fdc95e0ca18b2682c&to=10ec7c7960ea47e899003fcd7d1ba5c389ba1cae&stat=instructions

@jdoerfert It would be great if you could look over this as well.

Based on what @RalfJung mentioned on zulip, the question of whether the transform is legal for inbounds comes down to the particular choice of inbounds semantics. I was using the semantics specified in LangRef, which make the optimization illegal, while @nlopes used the semantics from https://people.mpi-sws.org/~jung/twinsem/twinsem.pdf (or something similar), which makes it legal. The relevant difference to the LangRef semantics (if we stick to the gep-inbounds-logical case) would be:

@bjope There are two more revisions based on this one, D86079 implements improved X86 lowerings and is already accepted, and D86078 implements improved AArch64 lowerings. Do you plan to land these as well?

LGTM. I'm not an expert on the pass manager, but after looking into it a bit this seems like the right direction. I don't think not preserving AAResults in HexagonLoopIdiomRecognition/GVNHoist is much of a problem. Preserving AAResults is generally not important (this "analysis" is essentially free) and these are non-default passes anyway.

LGTM, but please wait for @jdoerfert to check this as well.