Bug 237515
| Summary: | textproc/htmldoc hangs at runtime when compiled with clang 8 | ||
|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Tobias Kortkamp <tobik> |
| Component: | Individual Port(s) | Assignee: | freebsd-toolchain (Nobody) <toolchain> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | dim, jose |
| Priority: | --- | Keywords: | needs-qa, regression |
| Version: | Latest | Flags: | bugzilla:
maintainer-feedback?
(jose) |
| Hardware: | Any | ||
| OS: | Any | ||
| See Also: |
https://github.com/michaelrsweet/htmldoc/issues/349 https://bugs.llvm.org/show_bug.cgi?id=41998 |
||
|
Description
Tobias Kortkamp
2019-04-24 06:50:14 UTC
I can reproduce the issue, and it seems to be caused by a loop optimization after https://reviews.llvm.org/rL346397. There is an upstream PR, https://bugs.llvm.org/show_bug.cgi?id=39673, which is maybe related. I managed to reduce the issue to a fairly small test case, and submitted a new upstream bug: https://bugs.llvm.org/show_bug.cgi?id=41998 A commit references this bug: Author: dim Date: Mon Jul 1 21:06:11 UTC 2019 New revision: 349583 URL: https://svnweb.freebsd.org/changeset/base/349583 Log: Pull in r360968 from upstream llvm trunk (by Philip Reames): Clarify comments on helpers used by LFTR [NFC] I'm slowly wrapping my head around this code, and am making comment improvements where I can. Pull in r360972 from upstream llvm trunk (by Philip Reames): [LFTR] Factor out a helper function for readability purpose [NFC] Pull in r360976 from upstream llvm trunk (by Philip Reames): [IndVars] Don't reimplement Loop::isLoopInvariant [NFC] Using dominance vs a set membership check is indistinguishable from a compile time perspective, and the two queries return equivelent results. Simplify code by using the existing function. Pull in r360978 from upstream llvm trunk (by Philip Reames): [LFTR] Strengthen assertions in genLoopLimit [NFCI] Pull in r362292 from upstream llvm trunk (by Nikita Popov): [IndVarSimplify] Fixup nowrap flags during LFTR (PR31181) Fix for https://bugs.llvm.org/show_bug.cgi?id=31181 and partial fix for LFTR poison handling issues in general. When LFTR moves a condition from pre-inc to post-inc, it may now depend on value that is poison due to nowrap flags. To avoid this, we clear any nowrap flag that SCEV cannot prove for the post-inc addrec. Additionally, LFTR may switch to a different IV that is dynamically dead and as such may be arbitrarily poison. This patch will correct nowrap flags in some but not all cases where this happens. This is related to the adoption of IR nowrap flags for the pre-inc addrec. (See some of the switch_to_different_iv tests, where flags are not dropped or insufficiently dropped.) Finally, there are likely similar issues with the handling of GEP inbounds, but we don't have a test case for this yet. Differential Revision: https://reviews.llvm.org/D60935 Pull in r362971 from upstream llvm trunk (by Philip Reames): Prepare for multi-exit LFTR [NFC] This change does the plumbing to wire an ExitingBB parameter through the LFTR implementation, and reorganizes the code to work in terms of a set of individual loop exits. Most of it is fairly obvious, but there's one key complexity which makes it worthy of consideration. The actual multi-exit LFTR patch is in D62625 for context. Specifically, it turns out the existing code uses the backedge taken count from before a IV is widened. Oddly, we can end up with a different (more expensive, but semantically equivelent) BE count for the loop when requerying after widening. For the nestedIV example from elim-extend, we end up with the following BE counts: BEFORE: (-2 + (-1 * %innercount) + %limit) AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>) This is the only test in tree which seems sensitive to this difference. The actual result of using the wider BETC on this example is that we actually produce slightly better code. :) In review, we decided to accept that test change. This patch is structured to preserve the old behavior, but a separate change will immediate follow with the behavior change. (I wanted it separate for problem attribution purposes.) Differential Revision: https://reviews.llvm.org/D62880 Pull in r362975 from upstream llvm trunk (by Philip Reames): [LFTR] Use recomputed BE count This was discussed as part of D62880. The basic thought is that computing BE taken count after widening should produce (on average) an equally good backedge taken count as the one before widening. Since there's only one test in the suite which is impacted by this change, and it's essentially equivelent codegen, that seems to be a reasonable assertion. This change was separated from r362971 so that if this turns out to be problematic, the triggering piece is obvious and easily revertable. For the nestedIV example from elim-extend.ll, we end up with the following BE counts: BEFORE: (-2 + (-1 * %innercount) + %limit) AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>) Note that before is an i32 type, and the after is an i64. Truncating the i64 produces the i32. Pull in r362980 from upstream llvm trunk (by Philip Reames): Factor out a helper function for readability and reuse in a future patch [NFC] Pull in r363613 from upstream llvm trunk (by Philip Reames): Fix a bug w/inbounds invalidation in LFTR (recommit) Recommit r363289 with a bug fix for crash identified in pr42279. Issue was that a loop exit test does not have to be an icmp, leading to a null dereference crash when new logic was exercised for that case. Test case previously committed in r363601. Original commit comment follows: This contains fixes for two cases where we might invalidate inbounds and leave it stale in the IR (a miscompile). Case 1 is when switching to an IV with no dynamically live uses, and case 2 is when doing pre-to-post conversion on the same pointer type IV. The basic scheme used is to prove that using the given IV (pre or post increment forms) would have to already trigger UB on the path to the test we're modifying. As such, our potential UB triggering use does not change the semantics of the original program. As was pointed out in the review thread by Nikita, this is defending against a separate issue from the hasConcreteDef case. This is about poison, that's about undef. Unfortunately, the two are different, see Nikita's comment for a fuller explanation, he explains it well. (Note: I'm going to address Nikita's last style comment in a separate commit just to minimize chance of subtle bugs being introduced due to typos.) Differential Revision: https://reviews.llvm.org/D62939 Pull in r363875 from upstream llvm trunk (by Philip Reames): [LFTR] Rename variable to minimize confusion [NFC] (Recommit of r363293 which was reverted when a dependent patch was.) As pointed out by Nikita in D62625, BackedgeTakenCount is generally used to refer to the backedge taken count of the loop. A conditional backedge taken count - one which only applies if a particular exit is taken - is called a ExitCount in SCEV code, so be consistent here. Pull in r363877 from upstream llvm trunk (by Philip Reames): [LFTR] Stylistic cleanup as suggested in last review comment of D62939 [NFC] (Resumbit of r363292 which was reverted along w/an earlier patch) Pull in r364346 from upstream llvm trunk (by Philip Reames): [LFTR] Adjust debug output to include extensions (if any) Pull in r364693 from upstream llvm trunk (by Philip Reames): [IndVars] Remove a bit of manual constant folding [NFC] SCEV is more than capable of folding (add x, trunc(0)) to x. Pull in r364709 from upstream llvm trunk (by Nikita Popov): [LFTR] Fix post-inc pointer IV with truncated exit count (PR41998) Fixes https://bugs.llvm.org/show_bug.cgi?id=41998. Usually when we have a truncated exit count we'll truncate the IV when comparing against the limit, in which case exit count overflow in post-inc form doesn't matter. However, for pointer IVs we don't do that, so we have to be careful about incrementing the IV in the wide type. I'm fixing this by removing the IVCount variable (which was ExitCount or ExitCount+1) and replacing it with a UsePostInc flag, and then moving the actual limit adjustment to the individual cases (which are: pointer IV where we add to the wide type, integer IV where we add to the narrow type, and constant integer IV where we add to the wide type). Differential Revision: https://reviews.llvm.org/D63686 Together, these should fix a hang when building the textproc/htmldoc port, due to an incorrect loop optimization. PR: 237515 MFC after: 1 week Changes: head/contrib/llvm/include/llvm/Analysis/ValueTracking.h head/contrib/llvm/lib/Analysis/ValueTracking.cpp head/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp A commit references this bug: Author: dim Date: Fri Jul 12 19:44:00 UTC 2019 New revision: 349954 URL: https://svnweb.freebsd.org/changeset/base/349954 Log: MFC r349583: Pull in r360968 from upstream llvm trunk (by Philip Reames): Clarify comments on helpers used by LFTR [NFC] I'm slowly wrapping my head around this code, and am making comment improvements where I can. Pull in r360972 from upstream llvm trunk (by Philip Reames): [LFTR] Factor out a helper function for readability purpose [NFC] Pull in r360976 from upstream llvm trunk (by Philip Reames): [IndVars] Don't reimplement Loop::isLoopInvariant [NFC] Using dominance vs a set membership check is indistinguishable from a compile time perspective, and the two queries return equivelent results. Simplify code by using the existing function. Pull in r360978 from upstream llvm trunk (by Philip Reames): [LFTR] Strengthen assertions in genLoopLimit [NFCI] Pull in r362292 from upstream llvm trunk (by Nikita Popov): [IndVarSimplify] Fixup nowrap flags during LFTR (PR31181) Fix for https://bugs.llvm.org/show_bug.cgi?id=31181 and partial fix for LFTR poison handling issues in general. When LFTR moves a condition from pre-inc to post-inc, it may now depend on value that is poison due to nowrap flags. To avoid this, we clear any nowrap flag that SCEV cannot prove for the post-inc addrec. Additionally, LFTR may switch to a different IV that is dynamically dead and as such may be arbitrarily poison. This patch will correct nowrap flags in some but not all cases where this happens. This is related to the adoption of IR nowrap flags for the pre-inc addrec. (See some of the switch_to_different_iv tests, where flags are not dropped or insufficiently dropped.) Finally, there are likely similar issues with the handling of GEP inbounds, but we don't have a test case for this yet. Differential Revision: https://reviews.llvm.org/D60935 Pull in r362971 from upstream llvm trunk (by Philip Reames): Prepare for multi-exit LFTR [NFC] This change does the plumbing to wire an ExitingBB parameter through the LFTR implementation, and reorganizes the code to work in terms of a set of individual loop exits. Most of it is fairly obvious, but there's one key complexity which makes it worthy of consideration. The actual multi-exit LFTR patch is in D62625 for context. Specifically, it turns out the existing code uses the backedge taken count from before a IV is widened. Oddly, we can end up with a different (more expensive, but semantically equivelent) BE count for the loop when requerying after widening. For the nestedIV example from elim-extend, we end up with the following BE counts: BEFORE: (-2 + (-1 * %innercount) + %limit) AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>) This is the only test in tree which seems sensitive to this difference. The actual result of using the wider BETC on this example is that we actually produce slightly better code. :) In review, we decided to accept that test change. This patch is structured to preserve the old behavior, but a separate change will immediate follow with the behavior change. (I wanted it separate for problem attribution purposes.) Differential Revision: https://reviews.llvm.org/D62880 Pull in r362975 from upstream llvm trunk (by Philip Reames): [LFTR] Use recomputed BE count This was discussed as part of D62880. The basic thought is that computing BE taken count after widening should produce (on average) an equally good backedge taken count as the one before widening. Since there's only one test in the suite which is impacted by this change, and it's essentially equivelent codegen, that seems to be a reasonable assertion. This change was separated from r362971 so that if this turns out to be problematic, the triggering piece is obvious and easily revertable. For the nestedIV example from elim-extend.ll, we end up with the following BE counts: BEFORE: (-2 + (-1 * %innercount) + %limit) AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>) Note that before is an i32 type, and the after is an i64. Truncating the i64 produces the i32. Pull in r362980 from upstream llvm trunk (by Philip Reames): Factor out a helper function for readability and reuse in a future patch [NFC] Pull in r363613 from upstream llvm trunk (by Philip Reames): Fix a bug w/inbounds invalidation in LFTR (recommit) Recommit r363289 with a bug fix for crash identified in pr42279. Issue was that a loop exit test does not have to be an icmp, leading to a null dereference crash when new logic was exercised for that case. Test case previously committed in r363601. Original commit comment follows: This contains fixes for two cases where we might invalidate inbounds and leave it stale in the IR (a miscompile). Case 1 is when switching to an IV with no dynamically live uses, and case 2 is when doing pre-to-post conversion on the same pointer type IV. The basic scheme used is to prove that using the given IV (pre or post increment forms) would have to already trigger UB on the path to the test we're modifying. As such, our potential UB triggering use does not change the semantics of the original program. As was pointed out in the review thread by Nikita, this is defending against a separate issue from the hasConcreteDef case. This is about poison, that's about undef. Unfortunately, the two are different, see Nikita's comment for a fuller explanation, he explains it well. (Note: I'm going to address Nikita's last style comment in a separate commit just to minimize chance of subtle bugs being introduced due to typos.) Differential Revision: https://reviews.llvm.org/D62939 Pull in r363875 from upstream llvm trunk (by Philip Reames): [LFTR] Rename variable to minimize confusion [NFC] (Recommit of r363293 which was reverted when a dependent patch was.) As pointed out by Nikita in D62625, BackedgeTakenCount is generally used to refer to the backedge taken count of the loop. A conditional backedge taken count - one which only applies if a particular exit is taken - is called a ExitCount in SCEV code, so be consistent here. Pull in r363877 from upstream llvm trunk (by Philip Reames): [LFTR] Stylistic cleanup as suggested in last review comment of D62939 [NFC] (Resumbit of r363292 which was reverted along w/an earlier patch) Pull in r364346 from upstream llvm trunk (by Philip Reames): [LFTR] Adjust debug output to include extensions (if any) Pull in r364693 from upstream llvm trunk (by Philip Reames): [IndVars] Remove a bit of manual constant folding [NFC] SCEV is more than capable of folding (add x, trunc(0)) to x. Pull in r364709 from upstream llvm trunk (by Nikita Popov): [LFTR] Fix post-inc pointer IV with truncated exit count (PR41998) Fixes https://bugs.llvm.org/show_bug.cgi?id=41998. Usually when we have a truncated exit count we'll truncate the IV when comparing against the limit, in which case exit count overflow in post-inc form doesn't matter. However, for pointer IVs we don't do that, so we have to be careful about incrementing the IV in the wide type. I'm fixing this by removing the IVCount variable (which was ExitCount or ExitCount+1) and replacing it with a UsePostInc flag, and then moving the actual limit adjustment to the individual cases (which are: pointer IV where we add to the wide type, integer IV where we add to the narrow type, and constant integer IV where we add to the wide type). Differential Revision: https://reviews.llvm.org/D63686 Together, these should fix a hang when building the textproc/htmldoc port, due to an incorrect loop optimization. PR: 237515 Changes: _U stable/11/ stable/11/contrib/llvm/include/llvm/Analysis/ValueTracking.h stable/11/contrib/llvm/lib/Analysis/ValueTracking.cpp stable/11/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp _U stable/12/ stable/12/contrib/llvm/include/llvm/Analysis/ValueTracking.h stable/12/contrib/llvm/lib/Analysis/ValueTracking.cpp stable/12/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp |