This is an archive of the discontinued LLVM Phabricator instance.

Handle Top-Level-Regions in polly::isHoistableLoad
ClosedPublic

Authored by lksbhm on Nov 27 2017, 7:02 AM.

Details

Summary

This can be seen as a follow-up on my previous differential D33411.
We received a bug report where this error was triggered. I have tried my best to recreate the issue in a minimal lit testcase which is also part of this differential.

I only handle return instructions as predecessors to a virtual TLR-exit right now. From inspecting the codebase, it seems unreachable instructions may also be of interest here. If requested, I can extend my patches to consider them as well. I would also apply this on ScopHelper.cpp::isErrorBlock (see D33411), of course.

Diff Detail

Repository
rL LLVM

Event Timeline

lksbhm created this revision.Nov 27 2017, 7:02 AM
lksbhm updated this revision to Diff 124375.Nov 27 2017, 7:13 AM

Remove attribute from memcpy decl and add newline at end of file

lksbhm updated this revision to Diff 124378.Nov 27 2017, 7:31 AM

Remove FileCheck component from RUN line which caused the test to fail. Sorry

philip.pfaffe edited edge metadata.Nov 27 2017, 7:49 AM

LGTM.

@bollu: Can you verify that this fixes your issue?

Meinersbur added inline comments.
test/ScopDetect/tlr_is_hoistable_load.ll
1–3 ↗(On Diff #124378)

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.

lksbhm added inline comments.Nov 27 2017, 8:49 AM
test/ScopDetect/tlr_is_hoistable_load.ll
1–3 ↗(On Diff #124378)

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.

lksbhm updated this revision to Diff 124551.Nov 28 2017, 5:25 AM
  • Remove unneeded & duplicate polly arguments from the test run line
  • Re-add FileCheck with a few simple CHECK lines to the test
  • Add a description at the top of the test file
bollu edited edge metadata.Nov 28 2017, 5:35 AM

Other than request for documentation, LGTM. I did not test it, but I reduced the test case with bugpoint, so it should be the right fix. If not, I can complain again ;)

lib/Support/ScopHelper.cpp
475 ↗(On Diff #124551)

Could you please add a link to the test case? I'm not sure what the Polly convention on this is, but having a reference to what code path this covers would be helpful in the future :) Something like this is what I have in mind

bollu accepted this revision.Nov 28 2017, 5:35 AM
This revision is now accepted and ready to land.Nov 28 2017, 5:35 AM
philip.pfaffe added inline comments.Nov 28 2017, 5:58 AM
lib/Support/ScopHelper.cpp
475 ↗(On Diff #124551)

I really don't like the idea. Is there precedent for this in polly or llvm (besides the one @bollu posted)?

This revision was automatically updated to reflect the committed changes.