Visitation of declarations in FunctionAtrs with new and old pass managers?

I ran across a dependency in the way the new and old pass managers interact with function-attrs. I'm not sure whether this is expected behavior or not, but with some digging, I couldn't find a clear motivation. Anyone have context on this?

Essentially, under the old pass manager, FunctionAttrs appears to visit declarations, and under the new one, it doesn't. Here's an example that shows how this can change the output of function-attrs:

$ cat decl.ll

declare void @readnone() readnone

$ ./opt -S -function-attrs decl.ll -enable-new-pm=1
; ModuleID = 'decl.ll'
source_filename = "decl.ll"

; Function Attrs: readnone
declare void @readnone() #0

attributes #0 = { readnone }

$ ./opt -S -function-attrs decl.ll -enable-new-pm=0
; ModuleID = 'decl.ll'
source_filename = "decl.ll"

; Function Attrs: nofree nosync readnone
declare void @readnone() #0

attributes #0 = { nofree nosync readnone }

(The example uses nofree and nosync, but please don't focus on the semantics of those attributes. That's a separate discussion.)

To me, it seems odd to not have declarations be SCCs of their own, and thus passed to function-attrs. Does anyone have a good explanation for why we made this change? And in particular, what the "right" way of inferring attributes for a partially annotated declaration might be in our new world?

Philip

I think it makes more sense to do something like that in a pass like InferFunctionAttrsPass. We should decide to visit declarations consistently with function pass managers, which currently don’t visit declarations. Adding declarations to the new PM CGSCC infra would add complexity and passes would have to check if they’re working with a declaration vs definition.

Passes had to check declaration vs. definition before,
I don't think that is the most important argument.
That said, there is only so much you can do for declarations
anyway.

My proposal would be, as might have been expected, to run a

Passes had to deal with Decl. vs Def. before and they managed.
That said, there is only that much you can do with declarations
and I would suggest we do it early as part of a module pass.

Attributor run early as module pass will right now *not* annotate
the declaration but still derive the information for the call sites:
https://godbolt.org/z/b9a8Gheqb

We could make it annotate declarations as well with little effort.

~ Johannes

Arthur,

InferFunctionAttrs is currently a module pass and appears to be scheduled well before the CGSCC pass walk. I agree that we could add this logic there and get consistent behavior between the two pass managers. I’m a bit hesitant about having the logic for inference in two different places, but thinking about it, this doesn’t seem too bad. I couldn’t parse this statement, can you rephrase?

Passes had to deal with Decl. vs Def. before and they managed.
That said, there is only that much you can do with declarations
and I would suggest we do it early as part of a module pass.

This seems to align well with Arthur's suggestion, and seems reasonable to me as well.

Attributor run early as module pass will right now *not* annotate
the declaration but still derive the information for the call sites:
https://godbolt.org/z/b9a8Gheqb

We could make it annotate declarations as well with little effort.

Is Attributor a module pass? If not, you would seem to have the same problem as the existing code.

We should decide to visit declarations consistently with function pass managers, which currently don’t visit declarations.
I couldn’t parse this statement, can you rephrase?

I meant that currently function pass managers don’t run passes on declarations, so we should try to be consistent and have the CGSCC pass manager not run passes on declarations.

The new PM CGSCC infra assumes that only calls to known library functions can be introduced out of thin air. So we shouldn’t be introducing arbitrary declarations in a CGSCC pipeline (just intrinsics and library functions I believe).
I suppose it’s possible to introduce a call to an arbitrary function in some module pass, but that seems out of the ordinary.

This makes sense. It’s important to emphasize this is about the new pass manager. I don’t believe this matches the behavior of the old PM for either pass type.

The new PM CGSCC infra assumes that only calls to known library functions can be introduced out of thin air. So we shouldn’t be introducing arbitrary declarations in a CGSCC pipeline (just intrinsics and library functions I believe).

I suppose it’s possible to introduce a call to an arbitrary function in some module pass, but that seems out of the ordinary.

How does this connect? Everything you said parses; I just don’t see how this is connected to the topic of this thread.

Sorry, my point was that having an early module pass to infer attributes on existing declarations should be sufficient, we shouldn’t have to worry about doing that again later on in the pipeline.

Ah, I see. In theory, I agree.

In practice, I see some pragmatic issues. The main concern I have is that new declarations of intrinsics or "known functions" aren't annotated inside the CGSCC iteration. At the moment, that is only done by the previous module pass. As a result, if some transform replaces a call from one intrinsic with a call to a "known function" with more restrictive memory semantics (say), then we will not be able to infer the more restrictive semantics for the caller of the intrinsic in the same CGSCC pass.

I don't know how much that hurts, but it seems like we're missing some opportunities here, solely because we chose to not visit declarations. To avoid confusion, let me clarify that this is not a difference in behavior with the old pass manager. It had the same problem as well. This particular issue is related but distinct from the one I started the thread about (which is inferring attributes from each other on non-known functions.)

Passes had to deal with Decl. vs Def. before and they managed.
That said, there is only that much you can do with declarations
and I would suggest we do it early as part of a module pass.

This seems to align well with Arthur's suggestion, and seems reasonable to me as well.

Posted for review here: https://reviews.llvm.org/D100400.