Allow multiple noinline instances with different arguments. #6687

DirtyHairy · 2024-06-20T08:34:29Z

Allow multiple no-inline - passes as suggested in #6646 .

kripken

Nice!

kripken · 2024-06-21T20:00:26Z

src/passes/NoInline.cpp

- name, "Usage usage: wasm-opt --" + name + "=WILDCARD");
+ std::string pattern =
+ passArg ? *passArg
+ : getPassOptions().getArgument(


I wonder if we can simplify this? Specifically that it accepts two mechanisms seems a little more complex than it would ideally be.

In fact --passname=ARG sets both of them, so I think we only need to check the former? But I might be missing something.

The reason why I kept the global mechanism is --pass-arg.

Ah, yes, I agree we want the global mechanism too. And that makes sense: there can be a global flag that multiple passes (even of different operations, not just instances of the same Pass) care about, but also each pass instance can have its own. But here, I think this can always read passArg rather than check passArg and maybe read getPassOptions().getArgument(..).

kripken · 2024-06-21T20:02:05Z

src/pass.h

@@ -47,6 +51,7 @@ struct PassRegistry {
 std::vector<std::string> getRegisteredNames();
 std::string getPassDescription(std::string name);
 bool isPassHidden(std::string name);
+ bool doesPassAllowMultipleInstancesWithArgs(std::string name);


Can we make this the behavior of all passes? I can't think of a problem that would occur, and that seems simpler, but again I might be missing something.

Sure. I introduced the flag only for strict backwards compatibility for the other passes (bail out if the pass is specified multiple times with different arguments), but I'd be more than happy to remove it 😏

Ok good, I don't think we need to bother with the case of passes passed different arguments before now (that was never meant to work). So if no tests break, then let's make that change.

OK. Before I go ahead and make those changes, just to make sure we have the same picture of how the result should look like: All passes will use their own private value of the option that shares their name, and only options that have a different name and thus are set via --pass-arg will be read from the global map.

Yes, that sounds right.

Ah, I think I wasn't clear enough before, or I've misunderstood you. To make sure we are on the same page, the issue is that we need the fallback from the pass's argument to the global argument, because of the two forms --foopass=FOO and --pass-arg=foopass@FOO?

I think we can get around that. It looks like optimization-options handles the first form, and tool-options the second, but if they were coordinated this could be fixed, and passes would only read from the one place.

Concretely, I'm suggesting this:

When we see --foopass=FOO then we add it to the pass instance as an argument.

When we see --pass-arg=foopass@FOO then we check if there is a pass for it.

If there is, we write it to that pass instance as an argument. If there is more than one appearance of the pass, set it for them all.

If there is no such pass, write it to the global store.

There is no need for a fallback here, since we always write to the pass instance when it has the name of a pass, and always to the global store when not.

That means some more processing in tool-options/optimization-options, but I think it's worth it. It would be simpler, and also backwards compatible except for corner cases with multiple invocations of the same pass (which I am ok with altering).

Yeah, I think we managed to misunderstand each other 😏

To make sure we are on the same page, the issue is that we need the fallback from the pass's argument to the global argument, because of the two forms --foopass=FOO and --pass-arg=foopass@FOO?

Exactly.

I think your suggestion would work well, although I'd suggest a minor change:

When we see --foopass=FOO we set the argument on the pass instance

When we see --pass-arg=foopass@FOO we check the pass list for foopass

If we find an instance of foopass we set the argument there and do not continue to look for other, earlier instances

If there is no instance we store the argument in the global list, take it from there when we add foopass after that and remove it from the global list afterwards

This way, --pass-arg=foopass will always affect the previous instance of foopass on the command line, except when it occurs before the first --foopass, in which case it will affect the first instance. To me, this is a little more consistent with how I would expect the UI to behave (instead of a single --pass-arg=foopass overriding all previous instances).

Interesting. I like that it affects just one instance. Maybe we can make that even simpler, though: --pass-arg=foopass@FOO can always "attach" to the last instance, as you suggested, but also (1) it is an error if that pass already has an arg, and (2) it is an error if there is no pass instance before. Those avoid some possible ambiguity I think?

I like that even better 😏 I'll make the necessary changes.

OK, done. I also rebased my code on the current main head. If we agree that this is the behaviour we want to stick with I can also add a few tests.

…s-arg only for additional arguments.

src/binaryen-c.cpp

kripken

Nice work!

src/binaryen-c.cpp

src/pass.h

src/passes/pass.cpp

src/tools/optimization-options.h

kripken · 2024-07-02T20:00:52Z

Also, the behavior we decided on should be documented somewhere - did I miss that in the code?

DirtyHairy · 2024-07-03T21:20:51Z

Also, the behavior we decided on should be documented somewhere - did I miss that in the code?

No, there isn't anything yet, but I agree, some documentation would be nice and helpful. Do you have a pointer what a good place would be?

kripken · 2024-07-03T21:23:06Z

The description of --pass-arg would be one user-facing place. For internal code documentation, in pass.h where the pass has its argument could be a good place.

DirtyHairy · 2024-07-04T19:36:27Z

Thanks 😏 I've added a few comments.

kripken

Great lgtm with one suggestion.

kripken · 2024-07-08T20:49:48Z

src/tools/tool-options.h

- "in the form KEY@VALUE",
+ "in the form KEY@VALUE. If a pass --somepass is specified "
+ "multiple times, --pass-arg=somepass will apply to the closest "
+ "instance to the left. All other options apply to all instances.",


This should also say "if KEY is the name of a pass, then [..]". Overall maybe:

in the form KEY@VALUE. If KEY is the name of a pass then it applies to the closest instance of that pass before us. If KEY is not the name of a pass then it is a global option that applies to all pass instances that read it (later appearances of such global options override earlier ones).

I like you suggestion better than mine and adopted it, although I removed the last part in parenthesis as the text already is quite long, especially when formatted into a column.

Looks good!

kripken · 2024-07-08T20:53:08Z

Oh, also please add something to CHANGELOG.md

DirtyHairy · 2024-07-10T18:42:24Z

Oh, also please add something to CHANGELOG.md

Done!

kripken · 2024-07-10T21:59:08Z

CHANGELOG.md

@@ -38,6 +38,12 @@ Current Trunk
 - The build-time option to use legacy WasmGC opcodes is removed.
 - The strings in `string.const` instructions must now be valid WTF-8.
 - The `TraverseCalls` flag for `ExpressionRunner` is removed.
+ - Passes can now be specified several times on the command line. The "main"


Passes could already be specified more than once - what changes here is that each such instance can get its own argument?

How about

Passes can now receive individual pass arguments, that is --foo=A --foo=B for a pass foo will run the pass twice (which was possible before) and will now run it first with argument A and second with B. --pass-arg=foo@BAR will now apply to the most recent --foo pass on the commandline, if foo is a pass (while global pass arguments - that are not the name of a pass - remain, as before, global for all passes).

kripken · 2024-07-10T22:00:57Z

I started the tests here. I think for them to pass you need to update the help text outputs, which you can do by running ./scripts/update_help_checks.py and committing that output.

DirtyHairy changed the title ~~Allow multiple noinline instances with different arguments. Resolves #6646~~ Allow multiple noinline instances with different arguments. Jun 20, 2024

DirtyHairy mentioned this pull request Jun 20, 2024

Allow multiple patterns for noinline #6646

Open

kripken reviewed Jun 21, 2024

View reviewed changes

DirtyHairy added 3 commits June 29, 2024 11:45

Allow multiple noinline instances with different arguments.

ca91ab2

Allow multiple invocations with arguments for all passes, allow --pas…

01a3165

…s-arg only for additional arguments.

Support setting pass main argument again with --pass-arg

e6334a3

DirtyHairy force-pushed the main branch from a12f18b to e6334a3 Compare June 29, 2024 13:08

Fix new occurence of getArgument, force new code to use Pass methods.

2f08298

kripken reviewed Jul 1, 2024

View reviewed changes

src/binaryen-c.cpp Outdated Show resolved Hide resolved

Fix braces.

980a7bd

kripken reviewed Jul 2, 2024

View reviewed changes

DirtyHairy added 2 commits July 3, 2024 23:14

Review suggestions.

7bf7166

Comment.

ab51dd7

Comments.

ad3a099

kripken reviewed Jul 8, 2024

View reviewed changes

Wording, changelog.

eb44b01

kripken reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple noinline instances with different arguments. #6687

Allow multiple noinline instances with different arguments. #6687

DirtyHairy commented Jun 20, 2024

kripken left a comment

kripken Jun 21, 2024

DirtyHairy Jun 21, 2024

kripken Jun 21, 2024 •

edited

Loading

kripken Jun 21, 2024

DirtyHairy Jun 21, 2024

kripken Jun 21, 2024

DirtyHairy Jun 22, 2024

kripken Jun 24, 2024

kripken Jun 26, 2024 •

edited

Loading

DirtyHairy Jun 27, 2024

kripken Jun 27, 2024

DirtyHairy Jun 28, 2024

DirtyHairy Jun 29, 2024

kripken left a comment

kripken commented Jul 2, 2024

DirtyHairy commented Jul 3, 2024

kripken commented Jul 3, 2024

DirtyHairy commented Jul 4, 2024

kripken left a comment

kripken Jul 8, 2024

DirtyHairy Jul 10, 2024

kripken Jul 10, 2024

kripken commented Jul 8, 2024

DirtyHairy commented Jul 10, 2024

kripken Jul 10, 2024

kripken commented Jul 10, 2024

Allow multiple noinline instances with different arguments. #6687

Are you sure you want to change the base?

Allow multiple noinline instances with different arguments. #6687

Conversation

DirtyHairy commented Jun 20, 2024

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken left a comment

Choose a reason for hiding this comment

kripken commented Jul 2, 2024

DirtyHairy commented Jul 3, 2024

kripken commented Jul 3, 2024

DirtyHairy commented Jul 4, 2024

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jul 8, 2024

DirtyHairy commented Jul 10, 2024

Choose a reason for hiding this comment

kripken commented Jul 10, 2024

kripken Jun 21, 2024 •

edited

Loading

kripken Jun 26, 2024 •

edited

Loading