[PATCH] D40492: Handle Top-Level-Regions in polly::isHoistableLoad

[Lukas Böhm via Phabricator via llvm-commits]

lksbhm added inline comments.


================
Comment at: test/ScopDetect/tlr_is_hoistable_load.ll:1-3
+; RUN: opt %loadPolly -analyze -polly-scops  -polly-process-unprofitable \
+; RUN:     -polly-invariant-load-hoisting -polly-scops                   \
+; RUN:     -polly-detect-full-functions < %s
----------------
Meinersbur wrote:
> It would be nice to have a line that describes what this is testing. We usually also add `FileCheck`/`CHECK` lines to verify the expected output.
Thank you for your feedback! Yes, I would also like to use FileCheck here, but the expected output is pretty boring - just a bunch of "Invalid Scop!" lines. That's why I actively removed the FileCheck call (see reason for second update). If you think this would still be beneficial, I have no objections to adding some `CHECK` lines, though. 

Regardless of that, a small comment at the top of the file describing the intention of the test case sounds like a good idea. But I find it difficult to come up with a meaningful description aside from that I want to cover the code path where the error occurs. Maybe the description should focus on the exotic combination of cli options this test is run with?

This also leads me to the question if `ScopDetect` is the best location for this test. I chose it because from the call stack in gdb, `ScopDetection` was the pipeline stage at which the call to `isHoistableLoad` is made. If you have recommendations for a better location, I would be glad to hear them and move the test file accordingly.


https://reviews.llvm.org/D40492