[PATCH] D32739: [Polly] JSONImporter misses checks whether the data it imports makes sense.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

For some reason, I cannot apply the patch. The error I get:

  Checking patch dev/null => test/JSONExporter/ImportAccesses/ImportAccesses-Bad-relation.ll...
  error: dev/null: does not exist in index

The patch is also not based on the latest trunk anyway (CMakeLists.txt has changed). Please rebase to current trunk. Also, please add more context. as explained here; http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface . Please also update the summary.

Thank you for the update. Except for that I cannot apply the patch and try out, this looks very good.



================
Comment at: lib/Exchange/JSONExporter.cpp:475
         NewOutId = isl_map_get_tuple_id(NewAccessMap, isl_dim_out);
+
         auto *SAI = S.getArrayInfoByName(isl_id_get_name(NewOutId));
----------------
Please avoid changing unrelated parts. It's just noise in the diff.


================
Comment at: lib/Exchange/JSONExporter.cpp:520
           isl_set_free(CurrentAccessSet);
-
+          // Check if the JScop file changes the accessed memory.
           if (!IsSubset) {
----------------
Please keep the empty line, if there was one originally. Most of the time, we have an empty line before before comment lines.


================
Comment at: lib/Exchange/JSONExporter.cpp:624
   }
-
+  // Check if key 'type' differs from the current one or is not valid.
   SAI->getElementType()->print(RawStringOstream);
----------------
Please add a newline


================
Comment at: lib/Exchange/JSONExporter.cpp:688-691
+    for (unsigned i = 0; i < Arrays[ArrayIdx]["sizes"].size(); i++) {
       DimSizes.push_back(std::stoi(Arrays[ArrayIdx]["sizes"][i].asCString()));
+    }
+
----------------
There is no effective change here. Please keep the original version.


================
Comment at: test/JSONExporter/ImportSchedule/ImportSchedule-Wrong-number-statements.ll:5
+;
+; ; Verify if the JSONImporter check if there is the right number of statements.
+;
----------------
Redundant `;`


https://reviews.llvm.org/D32739