nit: remove _ suffix, i dont htink that follows naming convention

What was the context of this ?

My bad on this one during rebase.
The comment should be added when we add a second pass to insert profile_ivalue nodes for nvfuser https://github.com/pytorch/pytorch/pull/47668/files#diff-4b6a31b50172ed0fce36027a913fab9e59e496954264ad5e021324c74355a9c2R498-R500

cc'ing @Krovatkin

@eellison
If I remember correctly if we run InsertProfileNodesForCUDAFuser first it will insert a profile node on a list input (which is also a graph input) to aten::grad_sum_to_size which would in turn break the logic in InsertProfileNodesForSpecializeAutogradZero that checks that there's a direct use of the graph input that is also aten::grad_sum_to_size : https://github.com/pytorch/pytorch/pull/47667/files#diff-d606514ce9e63ab23356691f3f6df0616222cd50b8a41239add4601f14a53392R16

Ideally, it would be nice if different passes could insert profile nodes in whatever order, but I wasn't quite convinced of the merit of that approach, so I just decided to document it.

Could we remove all references to profile_optional ?

Will do.

The reason profile_optional was inserted on def and not per use is because we were just checking whether or not a value is None, which stays constant for each use.

General ivalue profiling doesn't have the same property, since you could profile mutable values like List which change over each use. So, this comment will be out of date once we have more uses of profile_ivalue.

i also wonder whether there's some value in sharing implementation, but having a different node name. depends i guess on the number of different ivalue profiling we have

I agree that we need to update the comment (and the checks as well?)

sharing implementation, but having a different node name
I think this is suggesting to have profile_ivalue as an extension to profile_optional, which is similar to what I have in the original PR prior to Nick's refactor.

IIUC, the refactor is to have a unique profile_ivalue node, where we would hopefully work towards a coherent profiling strategy that different passes relying on deploying/exploiting profiling ivalues could work consequently with each other. Granted that this is not what we have today, at least the order of passes has to be arranged in certain way to work. More so that it's tricky to cover all the corner cases.

I think the suggested middle ground to have different node name could make it easier to prevent profiling nodes inserted by different passes to breach into each other's territory for now. And we can merge them at later point when we figure out a universal profiling strategy.

@eellison

i also wonder whether there's some value in sharing implementation, but having a different node name. depends i guess on the number of different ivalue profiling we have

at the very least, it would give a better readability and slightly easier check since you only have to look for a specific symbol (rather a symbol + attribute)

On the other hand, the way we are adding profiling nodes, switching to different node names would result in a small explosion of copy-pasted classes in ir.h.
Also, if we decide in future to merge profiling node chains into a single profiling node with all the attributes, I think, the benefits of having just one single node on a use outweighs differently named nodes.
I would hold off on this approach.

@jjsjann123 @eellison

Granted that this is not what we have today, at least the order of passes has to be arranged in certain way to work

I would like to emphasize that this is no way the limitation of the current design! We could update how SpecializeAutogradZero inserts profiling nodes to account for a possibility that some other pass might have already inserted profiling nodes in between. Given we only have 3 passes that insert profiling nodes I decided to just document the order.

If we can figure out a way to have a different node-naming without a ton of copy-pasta, that's the best. I don't really know how to do that; maybe someone really good at C++ can.

Also, if we decide in future to merge profiling node chains into a single profiling node with all the attributes, I think, the benefits of having just one single node on a use outweighs differently named nodes.

i don't think this is coming anytime soon (if at all). I don't think we should over-consider this.

Granted that this is not what we have today, at least the order of passes has to be arranged in certain way to work

I think most things should be profiling per-use, which composes pretty well. specialize_autograd_zero is pretty unique

nit: getUseWithAttribute doesn't fully reflect the implementation, since we're also filtering out non profile_ivalue uses

I'm not convinced that changing removeProfiledOptionalUses(inp) to after guardSpecializations doesn't have some edge-case effect somewhere. Could we keep the code here as it was to minimize changes ?

I think there were multiple reasons why I moved it after guardSpecializations. One of them was that there could be arbitrary long chains of prim::profiles on some graph inputs. Even for a very simple case I considered where **both ** TE and SpecializeAutogradZero want to profile the optional list argument to aten::grad_sum_to_size, rewriting removeProfiledOptionalUses looked very ugly and I hit some edge cases there.
In contrast, walking a graph and removing after guardSpecializations is done looked so much simpler and it also looked quite similar to what we do elsewhere.

I tested this with fastrnns and our CI tests and didn't hit any regressions or crashes.
If you feel very strongly that removeProfiledOptionalUses(inp) should stay in this specific spot, we could try figuring it out.

If "rewriting removeProfiledOptionalUses looked very ugly" that is a good sign that pass writing is becoming more difficult as a result of the overloaded profile_ivalue cc @jjsjann123.

i don't feel that strongly, but I do prefer it as it was. IMO when there isn't a large overhead of doing so, we should be cleaning the graph as we are manipulating it in the pass. It's the same reason I added

The logic right now isn't quite right, either.

def foo(x):
   x = profile_ivalue[None=1, Present=1]
   return x

Here we won't insert any specializations, so the profile_ivalue nodes will remain in the graph. Granted this isn't likely to happen, but it's sort of a fallout of delaying logic past when it needs to be.

I am maybe more opinionated about this than I should be because in the past I've had to deal with passes that had incorrect IR (like when we never updated the requires_grad property) or other passes that similarly left things in confusing states throughout the pass.