Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New scheduling directive to disallow partitioning. #7882

Closed
wants to merge 2 commits into from

Conversation

mcourteaux
Copy link
Contributor

@mcourteaux mcourteaux commented Oct 6, 2023

Solves #7881.
The changes are numerous but always the same. In summary:

  • Just pass in the flag that sets allowing partitioning from the scheduling Dim to the For IR.
  • For::make() takes extra argument allow_partitioning.
  • All Lowing passes are updated to just pass this flag along or set it appropriately.
  • Make an additional .disallow_partitioning(VarOrRVar) function in Stage.

I will start working with this now, but this can be reviewed. Leaving it at WIP for a while until I have tested some more with it.

@abadams
Copy link
Member

abadams commented Oct 6, 2023

After thinking about this, I agree that this should be a scheduling primitive, and I agree with the way you've implemented it. We should think pretty hard about the name and interface though, because we're stuck with scheduling primitives forever. I'll flag this to discuss at the dev meeting so we can brainstorm the name. Let me know if you'd like an invite (assuming the time zone works for you). It's every other Friday at 1pm pacific time, and the next one is next week on the inauspicious 13th. If you can't make it we'll report back here with our thoughts and we can continue the discussion in text form.

@abadams abadams added the dev_meeting Topic to be discussed at the next dev meeting label Oct 6, 2023
@@ -441,6 +441,10 @@ struct Dim {
* loop (see the DimType enum above). */
DimType dim_type;

/** Allows whether the PartitionLoops lowering pass is allowed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any additions here also require updates to serialization and deserialization code. I suggest just copying how some other field is handled (i.e. dim_type).

@mcourteaux
Copy link
Contributor Author

In general, one of my thoughts about this was that I'd probably prefer .allow_partitioning(), rather than to disallow it, but I implemented it like this for now to be backwards compatible with how scheduling works today. Even stronger, rather than "allowing", you might "instruct it", with a warning when it can't find a good partitioning split.

Regarding the dev meeting, sure, I'd be happy to attend that. For the timezone, 1pm pacific time is 10pm for me, so I could join every now and then when it's relevant. 🙂

@mcourteaux
Copy link
Contributor Author

Yeah, I should compile locally with serialization enabled. Haven't tried that yet...

@abadams
Copy link
Member

abadams commented Oct 7, 2023

Maybe we want an enum rather than a flag in case new strategies for partitioning come up in future. Initially it would be Never, Always (which would error or warn as you suggest), and Auto (the default). Then the call might be something like .partition(y, Partition::Never)... I don't like the "Partition::" part of that very much though.

I guess we could also have partition_always(y), partition_auto(y). partition_never(y) as sugar.

@mcourteaux
Copy link
Contributor Author

mcourteaux commented Oct 7, 2023

Sounds good. Perhaps PartitionPolicy with indeed Never, Always, Auto. The Policy mimics better the naming scheme of TailStrategy.

@abadams abadams removed the dev_meeting Topic to be discussed at the next dev meeting label Oct 18, 2023
@mcourteaux mcourteaux closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants