Skip to content

Fix symbol names used for outputting schedules into valid C++ source variable names#5717

Open
derek-inteon wants to merge 3 commits intohalide:mainfrom
derek-inteon:derek/fix_schedule_source_formatting
Open

Fix symbol names used for outputting schedules into valid C++ source variable names#5717
derek-inteon wants to merge 3 commits intohalide:mainfrom
derek-inteon:derek/fix_schedule_source_formatting

Conversation

@derek-inteon
Copy link

Since Funcs and other classes can have user defined names, the resulting strings may be invalid C++ when used directly for variable names and identifiers during the source code generation for schedules.

This PR adds a 'conform_name' around all variable names being outputted to replace invalid characters with an underscore, to make sure only valid C++ identifiers are outputted for the schedule source.

};

// Conform the given name into a valid C++ identifier (eg for dumping a Func/Var inside a schedule to a header)
std::string conform_name(const std::string &name, const std::string &prefix="_");
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't have anything to do with logging, and thus doesn't belong in ASLog.h

Copy link
Author

@derek-inteon derek-inteon Feb 8, 2021

Choose a reason for hiding this comment

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

Yes, that's true. I placed it here since it was a common header, and deals with formatting text, but it's not actually doing any logging.

Would you propose a new header/source file or some other location?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it's only used in the one source file LoopNest.cpp? I would just put it in there (in an anonymous namespace too).

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

@alexreinking alexreinking added this to the v12.0.0 milestone Feb 13, 2021
@steven-johnson
Copy link
Contributor

Where does this PR stand?

@steven-johnson
Copy link
Contributor

Monday Morning Review ping: Where does this PR stand?

@steven-johnson
Copy link
Contributor

Is this PR still active? If not, it will be closed.

@alexreinking alexreinking modified the milestones: v12.0.0, v13.0.0 Apr 16, 2021
@steven-johnson
Copy link
Contributor

Is this PR still active?

Copy link
Contributor

@mcourteaux mcourteaux left a comment

Choose a reason for hiding this comment

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

As this fix is rather ad-hoc, I think a more central place that legalizes all Func, Var and RVar names would be more reasonable, as that solves this issue for all auto-schedulers.

Also, there is already some sanitation happening here:

// Sanitize the names of things to make them legal source code.
schedule_source = src.str();
bool in_quotes = false;
for (auto &c : schedule_source) {
in_quotes ^= (c == '"');
if (!in_quotes && c == '$') {
c = '_';
}
}

So this PR definitely needs changing.

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.

5 participants