Cool, thanks and LGTM!

LGTM!! Thanks for tracking down this surprising fix to the PR!

In D78471#2033046, @craig.topper wrote:

In D78471#2033001, @pengfei wrote:

In D78471#2032415, @craig.topper wrote:

Is the problem here really that foldMemoryOperand doesn't know to preserve the post instr symbol?

Yes and no. It's the problem I wanted to fix firstly. But after I read the code of hardening, I think the spill is also a problem. Because that pass always load the function address to register at the beginning. I think the spilling violates the objective the pass does. Beside, according to calling conversation, there are volatile registers before calling. So I think it's not a cost if we pin the address register.

I think we should be calling NewMI->cloneInstrSymbols in the same spot we copy memory references in TargetInstrInfo::foldMemoryOperand. Or we shouldn't disable folding if any symbols are present. We're effectively cloning an instruction when we fold the memory and we should make sure we don't drop things when we do that.

LGTM, but one of these I think should be addressed here.

LGTM other than two nits here, this is really awesome!

As much as I'm not thrilled about the duplication, the number of comments I have below about the exact O1 pipeline sure make it seem valuable. Unsure how you feel about addressing these here, later, etc.

Wooot about finally having a test case! (Sorry for nit picking it a bit ....)

LGTM with the addition of skipping debug info as Eli suggests.

Might also be good to explain a bit of *how* this fixes the PR to the commit log (or bug) for posterity. It seemed to surprise both of us that this was the fix when talking through the code, I suspect a future reader may benefit from having a log of what went on here.

Really like the approach now. Pretty minor code nits below only. =D

Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test flow that ensures the pass builder DTRT?

Just wanted to say thanks for the performance data! I know it was hard to get, but it is really, really useful to help folks evaluate these kinds of changes with actual data around the options available.

I just want to mention here, that my comment on the original patch still stands and really needs to be addressed:

I'm seeing lots of updates to fix bugs, but no movement for many days on both my meta comments and (in some ways more importantly) James's meta comments. (And thanks Philip for chiming in too!)

In D68267#1733661, @jonpa wrote:

• CodeGen/X86/copy-eflags.ll

"X86 EFLAGS copy lowering" transforms the def-use lists of $eflags (to local def-uses) without updating the livein lists of successor blocks:

bb.1.bb1:
  ...
  $eflags = COPY %16:gr32
  JCC_1 %bb.3, 12, implicit $eflags

bb.2.bb1:
; predecessors: %bb.1
  successors: %bb.3(0x80000000); %bb.3(100.00%)
  liveins: $eflags

bb.3.bb1:
; predecessors: %bb.1, %bb.2
  successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
  liveins: $eflags
  %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
  MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
  %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
  JCC_1 %bb.5, 12, implicit $eflags

=>

After X86 EFLAGS copy lowering:

bb.1.bb1:
  TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
  JCC_1 %bb.3, 5, implicit killed $eflags

bb.2.bb1:
; predecessors: %bb.1
  successors: %bb.3(0x80000000); %bb.3(100.00%)
  liveins: $eflags

bb.3.bb1:
; predecessors: %bb.1, %bb.2
  successors: %bb.4(0x40000000), %bb.5(0x40000000); %bb.4(50.00%), %bb.5(50.00%)
  liveins: $eflags
  %2:gr8 = PHI %8:gr8, %bb.2, %0:gr8, %bb.1
  MOV8mr %9:gr32, 1, $noreg, 0, $noreg, %2:gr8 :: (volatile store 1 into %ir.ptr1)
  %20:gr32 = MOV32rm %10:gr32, 1, $noreg, 0, $noreg :: (volatile load 4 from %ir.ptr2)
  TEST8rr %23:gr8, %23:gr8, implicit-def $eflags
  JCC_1 %bb.5, 5, implicit killed $eflags

It seems that since X86FlagsCopyLoweringPass::rewriteCondJmp() does JmpI.findRegisterUseOperand(X86::EFLAGS)->setIsKill(true), it should be safe to assume that EFLAGS is not live out. In that case it should also follow that any successor block will not use a live-in value of EFLAGS. So any successor block should have EFLAGS removed of the live-in list, right?

I am not sure what is the right way to clear EFLAGS of the successor blocks live-in lists. Should it be done in rewriteCondJmp() only or are there other places also that this should be done?

Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.

(Just a reminder that we need to have both performance and code size numbers for this patch. And given that there are a few options, may need a few examples.)

Thanks for sending this out!

A few minor further tweaks.

More fixes from Manuel.

Thanks, updated.

Address feedback from Manuel.

Generally very happy with this. Would like Eric to check this as well to think about the C API and other things that we maybe should be doing while unbuilding this....

FWIW, the adjustments I'm suggesting around tightening the logic can easily be in a follow-up patch if you like. I think generally the code LGTM and I'd just like us to pin down exactly what changes we expect to happen w/ the handles as much as possible to avoid subtle latent bugs creeping in and never getting noticed.

(Tried to get this out last weekend, but was blocked by the Phab down time... Sorry about that ...)

Adding Alina so she is aware of the change and can comment if she spots anything I'm missing...

LGTM to fix folks broken by this.

This also makes sense to me why it would OOM. I suspect we're building a massive list, and at best we get lucky and they get de-duped later. At worst, this is actually fixing a serious functionality problem. We had the same thing w/ normal ASan before too. Since this pass doesn't need to be a function pass anyways, this seems totally fine. Thanks for tracking it down.

This idea isn't fundamentally flawed, its a good idea and something we've discussed many times.

FWIW, LGTM. Thanks for all the work figuring out the right way to manage this at each step, and seeing it through, I think this is really great.

In D65280#1647026, @joerg wrote:

Chandler, are you OK with getting the InstructionSimplify.h part in now, so that it can be merged into 9.0 and the rest follow separately?

Thanks to Joerg for some useful discussion on IRC -- there was a concern I hadn't thought about that is exactly right: we somewhat want this pass to minimally disrupt things but also to be reasonably self contained.

Some nits below. LGTM with those fixed!

This seems .... somewhat unfortunate. Adding Chris Bieneman to see if this is really the right thing to do or there are any other alternatives.

As we discussed in person, we should refactor this so that when we enable MemorySSA we actually check the the loop passes in question mnanage to preserve it.

Also LGTM for cherrypick to the release branch.

This approach is broken for another reason, which also motivated the LoopSink approach David mentioned.

This too LGTM, and again, thanks for driving this all the way through.

We should specifically call this out in release notes as well (before we forget) as a bunch of downstream people will discover it in LLVM 10.

One high level point that is at least worth clarifying, and maybe others will want to suggest a different approach:

In D65280#1614752, @joerg wrote:

Looking a bit more into the details. Chandler, you've originally suggested going with the LowerAtomic route and that actually does create code that fails the SDAG lowering if the pass is skipped, e.g. on ARM.

Generally, I do like the approach. Two high level comments:

LGTM! Thanks for fixing this nasty bug (and sorry I wrote it).

LGTM with two nits addressed, thanks!

LGTM

Just to make sure we're on the same page (and sorry I didn't jump in sooner)...

Sorry for the delay here.

LGTM, thanks so much for sticking through this, I know it was ... nontrivial!

Pointing out the (serious) bug in this change below.

The used thing still seems like there is an underlynig bug here. See below.

Since this review is ongoing, please revert the original patch to unblock people. There is no harm in reverting and landing with the fix once it is ready. =]

In D63155#1563275, @xur wrote:

In D63155#1563266, @chandlerc wrote:

I just think we also need to understand why *no other test failed*, and why the only test we have for doing PGO at O0 is this one and it could be "fixed" but such a deeply unrelated change....

We have special code to do PGO at O0 in old pass manager. This is not done in the new pass manager.

In D63155#1563240, @leonardchan wrote:

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass
MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

As an alternative, could I instead request that we remove the BackendUtil changes and just mark the runs in gcc-flag-compatibility.c with -fno-experimental-new-pass-manager. This way we could clarify that under the new PM, we shouldn't run PGO at -O0? If not, I'll revert this patch as is.

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass
MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));
which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

Ok, now I've made a full pass through this. Mostly I think the first thing to do is tighten up the design around the core run function template and how reducers work with it. Documenting that design in detail will be really helpful I think.

(Also, really sorry, I mashed send button too soon, I'm still going through the code. Feel free to start on anything i posted, but sorry for the very random subset of comments)

I'd suggest enhancing the main description to include an overview of the code structure and organization to help reviewers follow the implementation design here. Think of it like a mini design doc for the *implementation* itself.

Personally, I'd suggest the name llvm-reduce for the tool.

LGTM

FWIW, I think we can wait for a bug to be filed or report come in with some functional test case before we change any behavior here.

Eh, this seems close enough now. I'd like a better approach for the x86 builtins, but no idea what it will end up being.

See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline.

LGTM

(Will likely need more eyes than just mine -- RPATH is mostly a mystery to me...)

just a minor comment on one of these...

LGTM, again, really nice. Tiny tweak below.

LGTM, nice!

FWIW, this LGTM. We may want to tweak the behavior around flattening, but that can happen as a follow-up, and this seems to get us to a better state.

OMG, I'm so sorry, I had no idea that the tests would explode like that... Yeah, I don't think that's useful....

In D63156#1543937, @leonardchan wrote:

I did some more digging and found the following differences between PMs for each test, and they seem to all differ and can be fixed for different reasons.

LGTM. Bit annoying that we need to do this at O0, but fine...

Code change LGTM, but again, let's update the test to check both ways.

I understand the change to explicitly say -O2. I also understand the change to add an explicit -fno-experimental-new-pass-manager to a RUN line when we have another RUN line that explicitly uses -fexperiemntal-new-pass-manager.

Let's update the test to explicitly run w/ both PMs to make sure this keeps working. LGTM with that change.

Code change LGTM. Can you update at least one of the tests to explicitly run both PMs so that we'll notice if this breaks in some weird way? Feel free to submit with that change.

This really confused me. We shouldn't be seeing this kind of difference in the new PM.

This was a lot easier for me to understand too, thanks. Somewhat minor code changes below.

I think this is somewhat the wrong approach.

I think this ultimately needs to be split up into smaller patches. A bunch of these things can be landed independently. Here is my first cut at things to split out, each one into its own patch.

I would just change this to have the module pass loop over the functions -- that seems like it'll be much cleaner.

Any chance you can add a test that shows one of the problems where verifying MemorySSA fails because we get a "preserved" copy after a loop pass that doesn't actually preserve it?