This is an archive of the discontinued LLVM Phabricator instance.

[gicombiner] Allow disable-rule option to disable all-except-...
ClosedPublic

Authored by dsanders on Jun 15 2020, 3:27 PM.

Details

Summary

Adds two features to the generated rule disable option:

  • '*' - Disable all rules
  • '!<foo>' - Re-enable rule(s)
    • '!foo' - Enable rule named 'foo'
    • '!5' - Enable rule five
    • '!4-9' - Enable rule four to nine
    • '!foo-bar' - Enable rules from 'foo' to (and including) 'bar'

(the '!' is available to the generated disable option but is not part of the underlying and determines whether to call setRuleDisabled() or setRuleEnabled())

This is intended to support unit testing of combine rules so
that you can do:

GeneratedCfg.setRuleDisabled("*")
GeneratedCfg.setRuleEnabled("foo")

to ensure only a specific rule is in effect. The rule is still
required to be included in a combiner though

Diff Detail

Event Timeline

dsanders created this revision.Jun 15 2020, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 3:27 PM
Herald added a subscriber: wdng. · View Herald Transcript
dsanders edited the summary of this revision. (Show Details)Jun 15 2020, 5:48 PM

It's not clear to me what "'!foo-bar' - Enable rules from 'foo' to (and including) 'bar'" means. These have some kind of ordering?

It's not clear to me what "'!foo-bar' - Enable rules from 'foo' to (and including) 'bar'" means. These have some kind of ordering?

Yes, it's the order that they're listed in the Combiner declaration (after flattening into a list) so it's a depth-first walk of the original list. It's mostly supported because there's no reason to special case it such that rule names don't work in the range syntax.

The point here is to allow people to only enable specific rules, right?

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

The point here is to allow people to only enable specific rules, right?

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

+1.
For starters we should explicitly include the combines we want (or maybe the group) similar to what @paquette suggested.

The point here is to allow people to only enable specific rules, right?

Yes, the point of this change is to support that case. The existing option was to support debugging and bisecting by disabling rules until to see if a miscompile gets resolved and using that to pin the miscompile to a particular rule.

Maybe an interface like this would be a bit simpler?

--my_combiner-only-enable-rules="rule1,rule2,rule3,..."

The reason I didn't add a second option is because you'd have to define the interaction between them:
--my_combiner-only-enable-rules=rule1,rule2,rule3 --my_combiner-disable-rule=rule2 sounds like rule 1 and 3 should be the only ones enabled, but then
--my_combiner-disable-rule=rule2 --my_combiner-only-enable-rules=rule1,rule2,rule3 sounds like it's rule1, 2, and 3 enabled but that's not implementable with the CommandLine library AFAIK. You'd actually get rule1 and 3

In general I'd prefer not to introduce conflicting options, I'll take another look at CommandLine and see if there's anything we can use there.

To enable rules {x,y}, you'd need to disable the universe, and specify rules you want to enable with !, all in a command called disable-rule*. This sounds inverse of what's necessary and is far from ergonomic.
I see this as similar to if opt/llc run-pass invocation was done by disable universe of passes, and enable some passes with !.
Additionally for the most common and immediately useful case of writing tests, I don't expect any need to both enable and disable rules in the same invocation. IOW, for testing purposes, only use the enable opt, and for bisecting and other tools (for future), use the disable opt. Perhaps we should rename the opt to make it explicit that it's used for testing only and perhaps even we assert that both are not used simultaneously (or make it abundantly clear in the docs).
TLDR; I'm suggesting we optimize the interface for the most common and immediately useful case of testing, and worry about enabling and disabling simultaneously later on?

dsanders updated this revision to Diff 271242.Jun 16 2020, 4:18 PM

Add only-enable-rule and make it interact with disable-rule correctly.

This looks much better. Looks good to me.

This revision is now accepted and ready to land.Jun 16 2020, 4:39 PM
This revision was automatically updated to reflect the committed changes.