LGTM

Thank you for writing this down.

In D88062#2294780, @dmgreen wrote:

Is this to ease porting between gcc and clang?

This patch caused some failures (fullfp16-neg.s etc.) and seems changed the behavior of parsing "vmov.f16 s1,r2" for -fp16 from fail to pass. With this patch and -fp16, "vmov.f16 s1,r2" would be parsed to "vmov.16 s1,r2" which is defined to alias "vmov s1,r2".

In D88107#2291529, @aeubanks wrote:

Actually, doing verify-each in StandardInstrumentations seems much nicer. Not sure about how to do debugify-each though, filed https://bugs.llvm.org/show_bug.cgi?id=47633.

gentle ping...

LGTM. Thanks.

In D86360#2282199, @jamieschmeiser wrote:

@ychen, Why are the regexs not sufficient for testing? I would rather not "check the common sub-string of GCC's and MSVC's output" because it has already been sufficiently demonstrated that this is fragile in the test.

"check the common sub-string of GCC's and MSVC's output" is not fragile because we only support GCC/Clang/MSVC as the host compiler. For PassID, GCC and Clang produce similar if not identical results. Checking PassID for only one compiler is fragile yes.

In D86360#2279738, @jamieschmeiser wrote:

Respond to build/test problems: Remove extra ;, remove call to base dtor,
change tests to have regex for PassID and ModuleID

• update test to demonstrate the bug
• actually the bug could be fixed by reverting D47898

One more nit. Thank you!

@arsenm Thanks for the review!

• address feedback

Hello, any more concern I could address to move this forward? Thanks.

• How about using PreservedAnalyses run(Loop &, LoopAnalysisManager &, LoopStandardAnalysisResults &, LPMUpdater &, LoopNest &); for the interface. I think it should work better because it is still a loop pass interface.
• Add a test case for PassInstrument?

ping?

• Missed a move for returned AsmStreamer.

Thank you for the patch. I'll take a look over the weekend.

Sorry to get around to this late. Some comments. If possible, please revert and recommit the patch after we solve these remaining issues.

In D87057#2253920, @aeubanks wrote:

move to Transforms/Utils instead due to layering violation
IR should not depend on Analysis

One more comment. Please let another reviewer ack.

Thanks for getting this started.

In D87001#2252711, @tejohnson wrote:

I need to think through this some more, but a couple questions:

Was the clang self host that you tried regular or Thin LTO? If regular please also try ThinLTO.

Do you have a test case showing the failure you got? I think it will be important to add if at all possible.

In D87001#2252711, @tejohnson wrote:

I need to think through this some more, but a couple questions:

Was the clang self host that you tried regular or Thin LTO? If regular please also try ThinLTO.

I was testing the patch with regular LTO. Will try ThinLTO also.

• ready for review

In D85676#2245787, @asbirlea wrote:

Seems like a good idea. I'm curious if you saw any impact from this change.

ping?

In D86178#2244855, @aeubanks wrote:

In D86178#2243750, @ychen wrote:

It looks like belong to addFunctionSimplificationPasses. How about keeping it Function pass using initialization once?

The reason I made this a module pass instead of a function pass is because it adds function declarations when there is a call to EP.get(). I'm not sure if you're allowed to add function declarations in a function pass. If we were to try to parallelize function passes, that would be bad.
We could split the creation of all the functions in ARCRuntimeEntryPoints into a separate module pass, but that would have to be run right before this function pass and wouldn't help with adding it to a FunctionPassManager.

• address feedback

It looks like belong to addFunctionSimplificationPasses. How about keeping it Function pass using initialization once?

The paper link is not accessible. Could you please make it available somewhere else?

Gentle ping?

In D86005#2231214, @aeubanks wrote:

Assuming that LoopFullUnrollPass is always required to run on anything with llvm.loop.unroll.full, it doesn't make sense to make this part of the optnone pass instrumentation since we'd have to add the check to all the pass instrumentations, not just optnone. e.g. opt-bisect should never skip the pass either.

If llvm.loop.unroll.full is strong enough that it should override all existing and future pass skipping callbacks' decision, we could either just add it to all the relevant callbacks (probably adding some utility functions) or we just put it in PassInstrumentation::runBeforePass. I think it is more important to adhere to the separation of concerns and let pass do transformation while let PassInstrumentation handles all pass skipping. Another reason (not that it matters here) for PassInstrumentation to handle pass skipping is that it could maintain pipeline execution state while pass itself could not. i.e. you could say the third run of foo pass should be skipped.

Please let @asbirlea acknowledge this also.

This decreases the number of check-llvm failures under NPM from 913 to 752.

This is great!

• address feedback

gentle ping?

LGTM

In D86005#2222884, @aeubanks wrote:

In D86005#2221998, @ychen wrote:

This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.

Do you mean don't have the optnone pass instrumentation say that the pass should be skipped if it sees the proper metadata?

This is to say optnone does not work for these 3 passes all the time. Could we do this in the optnone callback by checking the metadata of the branch inst in latch block? That is precisely what we meant here.

LGTM.

• address feedback

In D68789#2216803, @etiotto wrote:

In D68789#2216266, @ychen wrote:

I have trouble finding the definition of LoopNestAnalysis::run.

The reason is that this code is not an analysis pass (it was at the start of code review but ppl felt that it would be better to make it a utility rather than an analysis proper).

• fix typo

I have trouble finding the definition of LoopNestAnalysis::run.

Hi, @arsenm @craig.topper, I'm not sure what additional reviewers I should add. Any suggestions?

• rebase. ready for review

Yeah, that should work. Thanks.

Hello, sorry for the late reply. I have some high-level questions

• What are existing and future loop passes/analyses that could benefit from using LoopNestPass?
• I'd feel more comfortable if existing loop pass/analysis managers could be extended to handle LoopNestPass, rather than have separate full-fledged managers. Implementation aside, this models the idea that loopnest is just a special kind of loop, not a fundamental different IR unit. Have you thought about the alternative design of adding new adaptors or extending existing loop managers to run/query/invalidate loopnest passes?
• I'm not sure how common loopnest analyses are or will be? What's your opinion?

In D85698#2208716, @dblaikie wrote:

Perhaps this should be built by composition with some kind of range transformer (not sure if we have one of those already - maybe we do?), rather than having explicit support in join?

• update test

Looks reasonable to me. Please let @asbirlea double-check.