This is an archive of the discontinued LLVM Phabricator instance.

Fix crashes with -fprofile-use + new pass manager in queens.c from the testsuite (bugzilla bug 33776)
AbandonedPublic

Authored by jtony on Sep 20 2017, 2:26 PM.

Details

Summary

(1) The problem was originally reported on bugzilla, for more context please see: https://bugs.llvm.org/show_bug.cgi?id=33776

(2) Steps to reproduce the bug :
the test case queens.c is under directory: test-suite/SingleSource/Benchmarks/McGill

With the old pass manager, it is OK:
clang -fprofile-use -fsave-optimization-record -foptimization-record-file=pgo.yaml -O2 queens.c

But with the new pass manager, it emits fatal error:

clang -fprofile-use -fsave-optimization-record -foptimization-record-file=pgo.yaml -fexperimental-new-pass-manager -O2 queens.c
fatal error: error in backend: LICM: OptimizationRemarkEmitterAnalysis not cached at a higher level

(3) What have been done in this patch:
The other places which use getCachedResult(), they check the pointer is not NULL. So probably we just need to check the ORE pointer is not NULL as well (the crash happens at line 203 in lib/Transforms/Scalar/LICM.cpp). Especially there is a comment in the source code which says the fatal error emitting when ORE pointer is NULL should probably be optional rather than required.

With the proper checks in this patch, the crash goes away. However, this may not be the eventual solution, if people have other better solutions for this issue, we are very interested to hear them.

Diff Detail

Event Timeline

jtony created this revision.Sep 20 2017, 2:26 PM
davide edited edge metadata.Sep 20 2017, 2:35 PM

Thanks for your contribution.
Some notes:

  1. Can you please update the patch to include context (see the doc) ?
  2. Can you please add a test? I attached one to the PR.
  3. Can you please explain the rationale behind this fix? I'm afraid it's just papering over the problem rather than solving it.
nemanjai edited edge metadata.Sep 20 2017, 2:50 PM
  1. Can you please explain the rationale behind this fix? I'm afraid it's just papering over the problem rather than solving it.

I am not at all sure how this is supposed to work, but it certainly seems messy to always do a NULL check before using ORE. Perhaps someone that really knows how this is supposed to work can comment on why there's a requirement that the ORE object be available in the cache (i.e. why we need to acquire it through getCachedResult() rather than getResult() and add it to the cache if it isn't already cached).

jtony edited the summary of this revision. (Show Details)Sep 21 2017, 10:13 AM
jtony abandoned this revision.Oct 2 2017, 6:10 PM

Thanks for your contribution.
Some notes:

  1. Can you please update the patch to include context (see the doc) ?
  2. Can you please add a test? I attached one to the PR.
  3. Can you please explain the rationale behind this fix? I'm afraid it's just papering over the problem rather than solving it.