I fear you'll need to plan at least for new pm right now. Otherwise your change will be obsolete in half a year. What is it really that LINK_X_INTO_TOOLS is supposed to do? Effectively bake in something that's consumable by -load or -load-plugin, right? So can we use that interface directly? Effectively, can we have statically built-in plugins, that prepopulate the plug-in loader mechanisms?

Unfortunately this will stop working as soon as we switch on the new pass manager. With that, there is no canonical initialize*Passes that you can call. Do you have a plan to support the new pass manager as well?

In essence, the infrastructure is a set of callbacks that you can register with PassBuilder. Your callbacks then get invoked at various points withing pipelines built by PB.

I might have fallen a bit out of the loop, but can you tell me a bit more details about the use case here? There is no in-tree support for new-PM backend passes currently, is this for an out-of-tree user?
What about all the other IRUnit nestings beyond function passes in module pipelines?

Nit aside, looks good!

LGTM.

To clarify that: Currently clients are offered two APIs to populate a PassManager: One that consumes PipelineElements and one that consumes a string. When you have a PipelineElement in hand, you could use both, because it's mostly just a StringRef! I'd like to avoid that ambiguity.

IMO it makes sense to separate parsing of the pipeline text and the pipeline tree more. Parsing the pipeline string into the tree of pipeline elements is an operation that happens once and as the very first thing, after which the string is not needed anymore. The PassBuilder could do that part, and then call the PipelineParser to further parse the tree. Otherwise this looks great!

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

What is your specific usecase for this? You have a custom PassManager which can contain, e.g., a function pass pipeline?

Okay. Any plans to improve it? On the other hand, I'm not sure about the impact. We'll probably have to get some profile data first.

This makes complete sense to me. There is currently no way to invalidate an analysis for a specific IRUnit instance only, right? So not preserving an analysis cross-SCC will invalidate it for all?

On final language nit, otherwise LGTM!

Just two inline nits.

Looks good!

Couple of nits inline, otherwise looks great!

CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.

True, but that's what it is doing right now. Do you see reason to change that?

Why not default to CreateInfoOutputFile inside of TimePassesHandler if OutputStream is unset? And then have a setStream(raw_ostream&) method to change it.

LGTM. Did you verify that there's no conflicting info in the first three chapters of the tutorial?

Do you suggest to get rid of the current scheme where we have a single output file for all kinds of "info data"?

No I don't, that should certainly be the default.

This change is focused purely on the stream that LLVM uses to report the data being collected.
No matter how many TimePassesHandlers you create (and yes, we do create TimePassesHandler one per thread-per-pipeline) they all emit into the same CreateInfoOutputFile() - either stderr or a single file.
I'm trying to solve a problem of a badly interleaved output, thats all.

You could set the output directly on the handler.

I don't think this is sufficient or goes into the right direction to support parallel info collection and output.

Keep in mind that for the new PM we're talking about an API that's much more powerful, because it's designed for arbitrary hooks.

Tiny final nit, otherwise looks great!

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

It's still missing the O0 part. I forgot adding it for msan and tsan, but we should do asan correctly from the start.

Sorry, there's something I missed, comment inline.

Add the default argument back.

Great! Just three more nits. Feel free to land after adressing them!

Landed it for you in r352972. Thanks!

This is starting to look great! Minor nits inline.

It would be good to check, since the bots won't! Otherwise this looks good.

I'm not sure what the current state of plugins on windows is. They were broken and disabled last time I worked on this, but that might've changed in the meantime! Worth checking.

One thing I missed.

Nice, we're getting somewhere! A lot of comments inline. Most importantly, your analysis is a function analysis, which it mustn't be, otherwise there's no gain from it at all.

This generally looks sane. What will happen on windows though? Will it silently fail?

I ran a benchmark using asan on a synthetic testcase made up of 10k functions and 10k globals, and it's clearly visible that GlobalsMD causes the performance problem. This is because during initialization it looks at all the globals registered in llvm.asan.globals, which is used to mark the globals coming from the frontend which are supposed to be instrumented. The function pass, however, doesn't use most of this, and accesses GlobalsMD only within AddressSanitizer::GlobalIsLinkerInitialized to check whether a global is dynamically initialized. This means we can fix this in two ways:

@delcypher Thanks for pointing it out! Just landed a revised version in rL351314.

Address comments and update two missing testcases.

Execute appendtoGlobalCtors lazily as well. Previously the inserted ctor was
still added multiple times to the global ctors list. Doing this lazily requires
knowing when the ctor is actually created, so I added a callback to handle the
insertion.

Insert the constructor and init functions only once per module.

Fix a comment.

Add missing license to TSan header.

Looks good to me, thanks!

Thanks! Sorry that I missed this.

Rebase (and drop changes from D56386 that snuck in).

Rebase onto D56386 and add testcase for DA invalidation

Cool! Down to bikeshed comments :)

I haven't gotten back to this yet, but will do so after looking into what may be required for also porting SafeStack and ShadowCallStack to new PM.

If I recall correctly, it was pointed out on the SafeStack thread that, since SafeStack is part of CodeGen, porting it is not possible right now.

@leonardchan Are you making any progress here?

Superseded by D55647.

Addressed comments and rebased on top of rL350219.

Follow @chandlerc's proposal for a much more narrow API.

That solves the default case, but I feel like the new three-argument case is still not much better than getGlobalVariable and an if.

I get the idea of trying to separate query and creation arguments to make their meaning explicit. But how do you expect this to be used? The default call (i.e., the one everyone is using right now), would then look like

M.getOrInsertGlobal("Global", Ctx.getIntTy(), [] { return new GlobalVariable("Global", Ctx.getIntTy()); });

That's super verbose, and to me at least it's much harder to read then just calling Module::getGlobalVariable in an if.

The extra arguments are not filters. The idea here is to basicly be a mostly-drop-in replacement for calling the GlobalVariable constructor.

In D55605#1340583, @glider wrote:

A couple of nits.
I also think the newly added classes should be marked as such, not as structs, but YMMV.

Absolutely :)

Address comments:

• Move init creator function next to the ctor function
• Use Module::getOrInsertGlobal instead of my own version, but modify it to fit sanitizer neads

Abandoning in favor of D55647.

Thanks, LGTM!

This looks mostly like a mechanical change now, so LGTM!

This generally looks fine, some code comments inline.

It depends on what you mean by task. There is no clean way for depending on changes between IR units. I.e. you can't express the requirement that two passes have to run in a specific order. But that's okay, implementing such requirements would incure insane complexity. Fortunately, if I recall correctly Asan oesn't need that! Correct me if I'm wrong.

I dropped MSanOptions over in D55647 (or didn't need it there), which will likely supersede this change here.

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

The invalid handle isn't good for accessing anything, but it can still identify a unit.

The IRUnit is invalid, but the handle still exists, right?

Thanks, this does LGTM!

What exactly does this change solve?