This is an archive of the discontinued LLVM Phabricator instance.

[Polly] fixed test cases after removing immediate dominator heuristic for error block detection.
ClosedPublic

Authored by chelini on Apr 4 2018, 11:08 AM.

Details

Summary

This patch removes the heuristic in

  • Polly :: lib/Support/ScopHelper.cpp

The heuristic forces blocks that directly follow a loop header to not to be considered error blocks.
It was introduced in r249611 with the following commit message:

This replaces the support for user defined error functions by a
heuristic that tries to determine if a call to a non-pure function
should be considered "an error". If so the block is assumed not to be
executed at runtime. While treating all non-pure function calls as
errors will allow a lot more regions to be analyzed, it will also
cause us to dismiss a lot again due to an infeasible runtime context.
This patch tries to limit that effect. A non-pure function call is
considered an error if it is executed only in conditionally with
regards to a cheap but simple heuristic.

In the code below CCK_Abort2() would be considered as an error block, but not CCK_Abort1() due to this heuristic.

for (int i = 0; i < n; i+=1) {
  if (ErrorCondition1)
    CCK_Abort1(); // No __attribute__((noreturn))
  if (ErrorCondition2)
    CCK_Abort2(); // No __attribute__((noreturn))
}

This does not seem useful. Checking error conditions in the beginning of some work is quite common. It causes a switch default-case to be not considered an error block in SPEC's cactuBSSN. The comment justifying the heuristic mentions a "load", which does not seem to be applicable here. It has been proposed to remove the heuristic.

In addition, the patch fixes the following test cases:

  • Polly :: ScopDetect/mod_ref_read_pointer.ll
  • Polly :: ScopInfo/max-loop-depth.ll
  • Polly :: ScopInfo/mod_ref_access_pointee_arguments.ll
  • Polly :: ScopInfo/mod_ref_read_pointee_arguments.ll
  • Polly :: ScopInfo/mod_ref_read_pointer.ll
  • Polly :: ScopInfo/mod_ref_read_pointers.ll

The test cases failed after removing the heuristic.

Diff Detail

Repository
rL LLVM

Event Timeline

chelini created this revision.Apr 4 2018, 11:08 AM

Could you add the removal of the heuristic to this patch? Thanks.

chelini updated this revision to Diff 141112.Apr 5 2018, 1:41 AM
chelini edited the summary of this revision. (Show Details)

Added the removal of the heuristic in lib/Support/ScopHelper.cpp to the patch.

chelini edited the summary of this revision. (Show Details)Apr 5 2018, 2:09 AM

Thanks for the update. Can you run make polly-update-format on it (there seems to be some trailing whitespace)? The buildbots check the source code formatting and "fail" if some irregularity is found.

For reviews in Polly:

  1. Add "pollydev" and "llvm-commits" to the subscribers
  2. Add the tag [Polly] in front of the title. (adding llvm-commits to subscribers triggers a mail to the corresponding mailing list. Adding the tag is useful s.t. one doesn't need to open the mail to see that it is a change in Polly)
  3. Use "rPLO" for repository.
chelini retitled this revision from fixed test cases after removing immediate dominator heuristic for error block detection. to [Polly] fixed test cases after removing immediate dominator heuristic for error block detection..Apr 6 2018, 9:32 AM
chelini set the repository for this revision to rPLO Polly.
chelini updated this revision to Diff 141369.Apr 6 2018, 9:34 AM

removed trailing whitespaces.

Thanks for the update. Can you run make polly-update-format on it (there seems to be some trailing whitespace)? The buildbots check the source code formatting and "fail" if some irregularity is found.

For reviews in Polly:

  1. Add "pollydev" and "llvm-commits" to the subscribers
  2. Add the tag [Polly] in front of the title. (adding llvm-commits to subscribers triggers a mail to the corresponding mailing list. Adding the tag is useful s.t. one doesn't need to open the mail to see that it is a change in Polly)
  3. Use "rPLO" for repository.

I addressed the review comments. "pollydev" and "llvm-commits" were already added as subscribers. Could you please double-check?

Meinersbur accepted this revision.Apr 6 2018, 1:46 PM

LGTM. Good work!

Yes, llvm-commits and pollydev are subscribers (I actually didn't check by myself before, just added as a general node).

Do you have commit rights yet? If not, I can commit for you (with an annotation: "Contributed-by: Lorenzo Chelini").

This revision is now accepted and ready to land.Apr 6 2018, 1:46 PM

LGTM. Good work!

Yes, llvm-commits and pollydev are subscribers (I actually didn't check by myself before, just added as a general node).

Do you have commit rights yet? If not, I can commit for you (with an annotation: "Contributed-by: Lorenzo Chelini").

I just checked, I do not have the commit rights yet. Please commit it for me. Thanks.

This revision was automatically updated to reflect the committed changes.