LGTM

NewPM use seems to be fine.
A few relatively minor comments that I would like to see addressed.

LGTM.
Other people had plenty of time to look at this.

I'm not sure if introducing base for standard instrumentation classes (Instrument/Instruments) is a good change or not.
What problem does it solve?
Anyway that part seems to be rather orthogonal to the checker itself.
I would like to have it separated - either introduce Instrument/Instruments thing before the checker and then use it,
OR implement the checker the same way as other standard instrumentations are done (just as separate members) and only then refactor it all into Instrument/Instruments.

it would be great if you could share the unreduced reproducer, to verify it is the same issue.

This is our original reproducer:

@fhahn, we also had a crash downstream on our fuzzer run, if you still need a reproducer I can try reducing our failure.

LGTM, thanks for cleaning this up!

Look good to me, but I would like to hear other opinions...

LGTM, please give others at least 24h to respond.

LGTM.

Looks good to me, though I would like somebody else to check if running loopSimplify instead of simply bailing out is appropriate here.

Folks, we have a problem with this fix - when running a build that dynamically links tools with libLLVM.so (LVM_LINK_LLVM_DYLIB) we have massive failures on llvm-readobj
which asserts because it now has a conflict between MC "-W" and readobj "-W" command line options.
Seems like moving DWARFStreamer.cpp into the library w/o first removing its

static mc::RegisterMCTargetOptionsFlags MOF;

was a mistake.

This appears to cause suboptimal codegen on a relatively simple pattern:

https://bugs.llvm.org/show_bug.cgi?id=45378

@lebedev.ri I am not sure what your logic implied here; maybe instead of removal of assert this check should be a part of if for profitability reasons.

In review where this assert was introduced in one of the previous versions of it there was a check instead of assert (see line 261+: https://reviews.llvm.org/D69617?id=227953)
I believe doing this check makes sense.

As an explanation why PassInstrumentation is a better choice - you do not need to manually extend all the individual pass managers,
so you never miss one. This particular change missed at least two of them: RepeatPass and DevirtSCCRepeatedPass.

I'm sorry for the late comment, but this should have really been done through the PassInstrumentation framework.
It looks like a classic use-case.

In D75404#1902311, @skan wrote:

In e-mail @reames wrote:

Er, I'm not sure this is correct. The concern is that boundary align requires the computation of the size of a fragment following the boundary align. Since the size may be influenced by the starting offset (e.g. align), the size of the boundary align must already be known. The circularity of the logic there seems likely to lead to issues, though I don't have a specific test case.

Though... I guess the way we use BA makes this "safe" as we know the instructions following must be either DataFragments or RelaxableFragments.

And unfortunately，I did find some test cases failed during runtime. I agree with reame, one difference between boundar align
and align fragment is that the size of boundary align depends on the following fragment while align not. Before finishLayout, we have to fix its size before we write boudary align and this has to be done in relaxBoundaryAlign. I feel sorry that D75203 may need to change some logic since this patch was reverted.

We also found this patch to cause problems with our downstream testing, though I do not have good reproducers as of right now.

In D74346#1877375, @apilipenko wrote:

In D74346#1873034, @davidxl wrote:

what is the use case of these options? Do hidden nodes create too much noise?

This is mostly for performance analysis. In our compiler we end up with many code paths resulting in deoptimization. These code paths are supposed to be cold and are not interesting when we are looking for the "fast path" performance. Filtering these has a nice effect of streamlining the CFG to the common path only.

Makes sense to add rationale like this as a paragraph to revision description.

In D73804#1852503, @hgreving wrote:

Hello, this switch intends to help our backend to software pipeline single basic block loops. The suppressed GVN opt is creating control flow in loops which we like to avoid. Thanks in advance for looking at the patch.

Curiously, just recently we had a similar case in our downstream pipeline, though we had a GVN splitting loop backedge and leading to a loss of a loop form (by making a non-exiting latch).
Solved by disabling load-pre in a separate responsible GVN instance, though this control looks more appropriate.

setting request-changes state to make it obvious that there are some questions here I would like to see answered.

unittest added that verifies exit code of successful loop-rotate

fixing return value on subsequent iterations

converting recursion into inner loop

Hmm... phabricator is good in white-space diffs elimination! :)

made this change off by default.

improving some comments

adding CHECK-LABEL to the test so it passes with lit

reintroduced default parameter for Legacy pass constructor
(it is necessary for PassSupport.h proper operations and not otherwise used)

reformatting after the last update

fixing NoMemDepAnalysis use in legacy pass

removed old pass entry

In D72732#1821154, @skatkov wrote:

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

updated comments

LGTM.

Is there any testing that does enable attributor?

Okey, I did find a fair amount of tests that run individual -attributor *or* -passes=attributor (with -attributor-disable=false).
Are there any near plans to enable attributor (setting -attributor-disable=false by default)?

So, this enables Attributor pass, but in fact has all its functionality disabled unless -attributor-disable is set to false.
Is there any testing that does enable attributor?
If yes, then I would like to see this testing being run with new pass manager as well.

Is there a particular reason for choosing this spot in pipeline?
I see that in legacy pipeline Attributor follows IPSCCP/CVP, and here it goes right before.

Overall I'm fine with this.
Bikeshedding-wise, getPIC seems to be overly cryptic to me.
Do you believe full name getPassInstrumentationCallbacks is too much?

In D70157#1776424, @skan wrote:

What if I insert explicit align(8) right *after* the sequence?

If your insert explicit .align 8 after the sequence, and the sequence doesn't has any branch to be aligned, the current solution won't change the sequence.

Well, I kinda figure that from the code behavior right now, however is it really by design or just happens to work now?
Seeing that assembler becomes very intelligent now I would rather have a strict guarantee similar to "hardcode" thing that you have here that protects my sequence
rather than relying on a fact that in current settings moving my label by 8 does not appear to be profitable to assembler.

In D70157#1775016, @annita.zhang wrote:

The point is that we have explicit requirement at the start and we have a lowering into 16-byte sequence that we need to be preserved exactly as it is.
Essentially what we need is a "protection" for this sequence from any changes by machinery that generates the binary code.
How can we protect a particular byte sequence from being changed by this branch aligner?

No, in general we can't. The current solution is based on assembler to insert prefix or nop before the cross (or against) boundary branches. It can only ensure the explicit alignment specified by directive, but not any implicit alignment. I don't think any fixup based on assembler can do it. On the other hand, any code sequence after the alignment directive or even just in a function has some kind of implicit alignment. It's hard for assembler to tell which implicit alignment to preserve. The preferred way is to use explicit alignment directive to specify it.

For your scenario, a NOP padding is more controllable. NOP padding will be inserted just before the branch instructions (or macro fusion branch instructions). So if there's no branches (or macro fusion branches) in your code sequence, there will be no NOP inserted.

What if I insert explicit align(8) right *after* the sequence?

In D70157#1774561, @skan wrote:

I could not reproduce the phenomenon that N-byte nop becomes (N+M) bytes with your example. So according to my understanding, I slightly modified your case. (If my understand is wrong, I hope you can point it out :-). )

    .text
    nop
.Ltmp0:
    .p2align 3, 0x90
    .rept 16
    nop
    .endr
.Ltmp3:
    movl  %eax, -4(%rsp)

In our case it was

andl $1, %eax

but it does not matter that much.

In D70157#1768310, @skan wrote:

fix the bug

https://bugs.llvm.org/show_bug.cgi?id=44215

FYI: I did close the bug as fixed after verifying that the fix works for me.

In D70157#1767212, @fedor.sergeev wrote:

Working on getting upstream reproducer....

Hangs on a moderately small piece (~150 lines) of x86 assembly, filed a bug on it here:

Just FYI for now as I'm trying to dig futher...
I have been trying this fix in our downstream environment and managed to get a hang with this backtrace:

The change you suggest, doesn't cause the crash.

would you mind to add that test here?
It should exercise a path of logic where argument comes through SimplifiedValues, so it is worth having it.

Can you add a test similar to what you have here that excercises indirect call through a parameter?
Say, do

define void @func1() {
  %t = bitcast void ()* @func3 to void ()*
  tail call void @func2(void()* %t)
  ret void
}
define void @func2(void()* %f) {
  tail call void %f()
  ret void
}

and then......

define void @func6() {
  %t2 = bitcast void (void()*)* @func2 to void (void()*)* 
  %t3 = bitcast void ()* @func3 to void ()*
  tail call void %t2(void()* %t3)
  ret void
}

This results in recursive call to func2+ and your fix should handle it just right.

llvm/test/Transforms/Inline/pr35469.ll

btw, I would rather have this test named by its functionality and not by PR number.

slightly modified test

Great, thanks a lot!

In D67701#1674862, @fedor.sergeev wrote:

I might be in need for the morning coffee, but I cant find the actual use of UP.FullUnrollMaxCount...

Ugh... I was looking for that use in the new code, but it is there in the old code, sorry.

I might be in need for the morning coffee, but I cant find the actual use of UP.FullUnrollMaxCount...

This reproducer fails with assertion:

] cat reduced.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:1"
target triple = "x86_64-unknown-linux-gnu"

LGTM.
Thanks for addressing this!

Which part of the basics specifically?

Also, avoid adding anything into the adaptor except just basic functionality of passing control to the target pass.
(say, as I commented already - skip remarks stuff altogether). This commit better contain the general comments on what MachineFunction is
and basics required for PassManager/Adaptor to start working, and nothing else.
Since you will have to add PassInstrumentation interfaces in order for PassManager to operate you might be able to add its usage into PassAdaptor as well.
But that could be done as a follow up change.

In D64179#1610998, @czhang wrote:

In D64179#1606205, @fedor.sergeev wrote:

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

Okey, everything you said there makes perfect sense. Thanks for clarification.