Page MenuHomePhabricator

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

Authored by niosega on May 2 2017, 3:53 AM.



Without this patch, the JSONImporter did not verify if the data it loads were correct or not (Bug 32543). I add some checks in the JSONImporter class and some test cases.

Here are the checks (and test cases) I added :


  • The "context" key does not exist.
  • The context was not parsed successfully by ISL.
  • The isl_set has the wrong number of parameters.
  • The isl_set is not a parameter set.


  • The "statements" key does not exist.
  • There is not the right number of statement in the file.
  • The "schedule" key does not exist.
  • The schedule was not parsed successfully by ISL.


  • The "statements" key does not exist.
  • There is not the right number of statement in the file.
  • The "accesses" key does not exist.
  • There is not the right number of memory accesses in the file.
  • The "relation" key does not exist.
  • The memory access was not parsed successfully by ISL.


  • The "type" key does not exist.
  • The "sizes" key does not exist.
  • The "name" key does not exist.

/!\ Do not check if there is an key name "arrays" because it is not considered as an error.
All checks are already in place or implemented in JSONImporter::areArraysEqual

Diff Detail


Event Timeline

niosega created this revision.May 2 2017, 3:53 AM
niosega created this object with edit policy "niosega (Bonfante Nicolas)".
niosega edited the summary of this revision. (Show Details)
niosega updated this revision to Diff 97431.May 2 2017, 4:43 AM

The first one is not complete.

Meinersbur edited edge metadata.May 2 2017, 8:47 AM

Please don't copy-paste my suggestions into the summary, especially not because they do not reflect what you actually implemented.

287 ↗(On Diff #97431)

Suggestion: "Check whether the context was parsed successfully."

335 ↗(On Diff #97431)


338 ↗(On Diff #97431)

not equals



605–608 ↗(On Diff #97431)

A SCoP doesn't strictly need arrays.

So far none of your tests has an arrays key, meaning all of them would fail if there is no error before.

626 ↗(On Diff #97431)

has not key

633 ↗(On Diff #97431)

index (lower case i)

1 ↗(On Diff #97431)

Window's implementation of llvm-lit doesn't handle this:

UNRESOLVED: Polly :: JSONExporter/rlr.ll (397 of 869)
******************** TEST 'Polly :: JSONExporter/rlr.ll' FAILED ********************
Exception during script execution:
Traceback (most recent call last):
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 187, in execute_test
    result = test.config.test_format.execute(test, lit_config)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\formats\", line 12, in execute
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 1082, in executeShTest
    res = _runShTest(test, litConfig, useExternalSh, script, tmpBase)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 1030, in _runShTest
    res = executeScriptInternal(test, litConfig, tmpBase, script, execdir)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 548, in executeScriptInternal
    exitCode, timeoutInfo = executeShCmd(cmd, shenv, results, timeout=litConfig.maxIndividualTestTime)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 137, in executeShCmd
    finalExitCode = _executeShCmd(cmd, shenv, results, timeoutHelper)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 245, in _executeShCmd
    res = _executeShCmd(cmd.lhs, shenv, results, timeoutHelper)
  File "C:\Users\Meinersbur\src\llvm\utils\lit\lit\", line 353, in _executeShCmd
    r[2] = open(redir_filename, r[1])
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\Meinersbur\\src\\llvm\\tools\\polly\\test\\JSONExporter\\rlr.ll)'

Note that the RUN line is not really a shell command, so () doesn't launch a sub-shell. llvm-lit has its own parser for them.

Try to avoid 2>&1 if possible. How two output streams are merged is undefined. Now I think there is no direct way to only to only FileCheck stderr, but you can disable stderr with --disable-output. Or you can use 2>&1 >/dev/null which is explicitly supported under Windows.

Thank you for the patch, it looks very promising.

When uploading patches, please use also add context (with git: git diff -U999999, see:

9 ↗(On Diff #97431)

Mmmh, that list looks very outdated. It is also only used to make polly-test-${dir} targets and only in out-of-LLVM-source builds. I think we should remove it (I'll do this in a separate commit)

2 ↗(On Diff #97431)

I don't think the -analyze output is interesting for these test cases.

simbuerg added inline comments.
382 ↗(On Diff #97431)


640 ↗(On Diff #97431)

has no key

649 ↗(On Diff #97431)

has no key

niosega updated this revision to Diff 97641.EditedMay 3 2017, 7:45 AM

This diff takes into account the remarks of Michael Kruse and Andreas Simbuerger.

Are you going to implement checks for importAccesses as well?

grosser edited edge metadata.May 10 2017, 11:50 AM

@niosega : Any updates here?

This comment was removed by niosega.
605–608 ↗(On Diff #97431)

There was already an if to check whether the Json contains a arrays key or not. I have just added an output to inform the user. Should I remove my output ?

I am currently working on adding checks for importAccess and cleaning the test cases (explain what they are doing, better naming .. )

Meinersbur added inline comments.May 11 2017, 4:27 AM
605–608 ↗(On Diff #97431)

Since it returns true, it looks like normal operation. Error output would be confusing. So, set, you can remove it.

niosega updated this revision to Diff 98977.May 15 2017, 4:20 AM
niosega edited reviewers, added: simbuerg; removed: Meinersbur, grosser.
niosega changed the visibility from "Public (No Login Required)" to "Custom Policy".
niosega removed subscribers: mgorny, llvm-commits, pollydev.
niosega updated this revision to Diff 99256.May 17 2017, 12:59 AM
niosega added reviewers: Meinersbur, grosser.
niosega changed the visibility from "Custom Policy" to "Public (No Login Required)".
niosega added subscribers: mgorny, llvm-commits, pollydev.
Meinersbur edited edge metadata.May 17 2017, 6:34 AM

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

475 ↗(On Diff #99256)

Please avoid changing unrelated parts. It's just noise in the diff.

520 ↗(On Diff #99256)

Please keep the empty line, if there was one originally. Most of the time, we have an empty line before before comment lines.

624 ↗(On Diff #99256)

Please add a newline

688–691 ↗(On Diff #99256)

There is no effective change here. Please keep the original version.

5 ↗(On Diff #99256)

Redundant ;

niosega updated this revision to Diff 99909.May 23 2017, 7:29 AM

Take into account the remarks of Tobias concerning the line before comments.
Also rebase with the last version of master.

niosega edited the summary of this revision. (Show Details)May 23 2017, 8:54 AM
Meinersbur accepted this revision.May 24 2017, 8:02 AM

Thank you. Especially for the many test cases and otherwise high quality.

I am going to commit it on your behalf in a moment.

This revision is now accepted and ready to land.May 24 2017, 8:02 AM
This revision was automatically updated to reflect the committed changes.