[PATCH] D33362: [Polly][WIP]JSCoP Importer: support for multi-valued schedules

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for the patch. The lexmin/subtract cycle look ok. Could you add a test case which I can try out and see what kind of input you generate and what you expect as output?

I added some remarks including some about the coding style we use.



================
Comment at: include/polly/ScopInfo.h:1115
+  
+  /// Create the ScopStmt from another ScopStmt
+  ScopStmt(ScopStmt *Original, int CopyNo);
----------------
We usually add dots add the end of comments/sentences.


================
Comment at: include/polly/ScopInfo.h:2092
+  /// Create a new SCoP statement for @p ScopStmt
+  void addScopStmt(ScopStmt *newStmt, int CopyNo);
+    
----------------
[[http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly|LLVM coding style]] says variables, including function parameters, should start with a capital letter.


================
Comment at: lib/Analysis/ScopInfo.cpp:1579
+	R(Original->getRegion()), Build(nullptr), SurroundingLoop(Original->getSurroundingLoop()){
+	BaseName = getIslCompatibleName((std::string) Original->getBaseName(), "_copy_no_", std::to_string(CopyNo));
+	Domain = Original->getDomain();
----------------
Cast `(std::string)` is unnecessary.


================
Comment at: lib/Analysis/ScopInfo.cpp:1581
+	Domain = Original->getDomain();
+	Domain = isl_set_set_tuple_name(Domain, BaseName.c_str());
+}
----------------
You must also change the user part of the `isl_id`.


================
Comment at: lib/Analysis/ScopInfo.cpp:4452
 
+void Scop::addScopStmt(ScopStmt *newStmt, int CopyNo) {
+  assert(newStmt && "Unexpected nullptr!");
----------------
Maybe call it `copyScopStmt`?


================
Comment at: lib/Exchange/JSONExporter.cpp:304
+// Adds a copy of the map corresponding to the set to the user->NewMap
+static isl_stat CopyScheduleStatements(__isl_take isl_set *set, void *user){
+  struct schedule_info *data = (schedule_info*) user;
----------------
In LLVM's coding style, function names start with a lower case letter.


================
Comment at: lib/Exchange/JSONExporter.cpp:348
+	P = isl_printer_flush(P);
+	P = isl_printer_print_union_map(P, ScheduleMap);
+	printer = isl_printer_get_str(P);
----------------
There is an overloaded `operator<<`, you can push it directly to `OS`.


================
Comment at: lib/Exchange/JSONExporter.cpp:364
+	//create a schedule containing only duplicate elements
+	FinalSchedule = isl_union_map_lexmin(isl_union_map_copy(ScheduleMap));
+	ScheduleDup = isl_union_map_subtract(isl_union_map_copy(ScheduleMap), isl_union_map_copy(FinalSchedule));
----------------
`isl_union_map_lexmin` can fail if the input is not lower-bounded. Could you add a check/assertion?

It might be easier to iterate over the Stmts first such that we get a `isl_map` and call `isl_map_lexmin`/`isl_map_subtract` on it.  An advantage is that one `Stmt` is handles at a time, helping to diagnose which of those is causes a problem.


================
Comment at: lib/Exchange/JSONExporter.cpp:367-371
+	// create struct to be used by the isl_union_set_foreach_set functions 
+	struct print_info ScopData = {&S};
+	struct schedule_info ScheduleData;
+	ScheduleData.OriginalUmap = isl_union_map_copy(FinalSchedule);
+	ScheduleData.NewUmap = isl_union_map_copy(FinalSchedule);
----------------
Could you consider using `isl::union_map::foreach_map`? This allows using C++11-style closures with less boilerplate code.


================
Comment at: lib/Exchange/JSONExporter.cpp:374
+
+	while(!isl_union_map_is_empty(ScheduleDup)){
+		universe = isl_union_map_universe(isl_union_map_copy(ScheduleDup));
----------------
Sequential lexmin+subtract is not guaranteed to eventually end up in an empty set. Could you add a cut-off maximum number of iterations?


================
Comment at: lib/Exchange/JSONExporter.cpp:382
+					&CopyScopStatements, &ScopData) < 0)
+			errs() << "error during foreach\n";
+
----------------
Consider using `assert` for such checks.


================
Comment at: lib/Exchange/JSONExporter.cpp:398
+
+	print_union_map("New Map:", P, errs(), ScheduleData.NewUmap);
+
----------------
There is `isl_union_map_dump(ScheduleData.NewUmap)`.


================
Comment at: lib/Exchange/JSONExporter.cpp:433
 
   if (!D.isValidSchedule(S, &NewSchedule)) {
     errs() << "JScop file contains a schedule that changes the "
----------------
Does this function properly handle non-right-unique schedules? 


================
Comment at: lib/Exchange/JSONExporter.cpp:451
+  if(!isl_union_map_is_single_valued(ScheduleMap)){
+	errs() << "JScop file contains a schedule that is not single valued\n";
+
----------------
Don't print errors during normal operation. Use
```
DEBUG(dbgs() << "JScop file contains a schedule that is not single valued\n");
```
for messages that helps you during development.


https://reviews.llvm.org/D33362