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

[Mike via Phabricator via llvm-commits]

Mike_Jongen updated this revision to Diff 99899.
Mike_Jongen added a comment.

In https://reviews.llvm.org/D33362#760795, @Meinersbur wrote:

> 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.


Thank you for the comments. I fixed most remarks.
I found an error in the new schedule. After I fix this I will add a test case.




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?

Not sure if I understand. How can I check this, is there a function for this?

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.

Good Idea, I will have a look at it.




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.

I will have a look at it. I used foreach_set because this was also used by the function which checks if a map is single valued, but foreach_map might be better here indeed.




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?

It does not at the moment. I use disable-legality to go pass this check, I will have a look at it later.


https://reviews.llvm.org/D33362

Files:
  include/polly/ScopInfo.h
  lib/Analysis/ScopInfo.cpp
  lib/Exchange/JSONExporter.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33362.99899.patch
Type: text/x-patch
Size: 7327 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170523/31aacd3c/attachment-0001.bin>