Project Ortega BCM5719 LLVM https://git.raptorcs.com/git/bcm5719-llvm/atom?h=meklort-10.0.1 2019-09-29T15:25:24+00:00 2019-09-29T15:25:24+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-09-29T15:25:24+00:00 urn:sha1:d30093bb8a3f12d35d176a85cf93e354a38ff116 If we happen to have the same div in two basic blocks, and in one of those we also happen to have the rem part, we'd match the div-rem pair, but the wrong ones. So let's drop overly-ambiguous assert. Fixes https://bugs.llvm.org/show_bug.cgi?id=43500 llvm-svn: 373167 2019-07-31T12:06:51+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-31T12:06:51+00:00 urn:sha1:a686c60c45d516cc8870b77af97fa66e3578807d Summary: While `-div-rem-pairs` pass can decompose rem in div+rem pair when div-rem pair is unsupported by target, nothing performs the opposite fold. We can't do that in InstCombine or DAGCombine since neither of those has access to TTI. So it makes most sense to teach `-div-rem-pairs` about it. If we matched rem in expanded form, we know we will be able to place div-rem pair next to each other so we won't regress the situation. Also, we shouldn't decompose rem if we matched already-decomposed form. This is surprisingly straight-forward otherwise. The original patch was committed in rL367288 but was reverted in rL367289 because it exposed pre-existing RAUW issues in internal data structures of the pass; those now have been addressed in a previous patch. https://bugs.llvm.org/show_bug.cgi?id=42673 Reviewers: spatel, RKSimon, efriedma, ZaMaZaN4iK, bogner Reviewed By: bogner Subscribers: bogner, hiraditya, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D65298 llvm-svn: 367419 2019-07-31T12:06:38+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-31T12:06:38+00:00 urn:sha1:5f616901f579062aabe32fee165eacd36de82e8b Summary: `DivRemPairs` internally creates two maps: * {sign, divident, divisor} -> div instruction * {sign, divident, divisor} -> rem instruction Then it iterates over rem map, and looks if there is an entry in div map with the same key. Then depending on some internal logic it may RAUW rem instruction with something else. But if that rem instruction is an input to other div/rem, then it was used as a key in these maps, so the old value (used in key) is now dandling, because RAUW didn't update those maps. And we can't even RAUW map keys in general, there's `ValueMap`, but we don't have a single `Value` as key... The bug was discovered via D65298, and the test there exists. Now, i'm not sure how to expose this issue in trunk. The bug is clearly there if i change the map keys to be `AssertingVH`/`PoisoningVH`, but i guess this didn't miscompiled anything thus far? I really don't think this is benin without that patch. The fix is actually rather straight-forward - instead of trying to somehow shoe-horn `ValueMap` here (doesn't fit, key isn't just `Value`), or writing a new `ValueMap` with key being a struct of `Value`s, we can just have an intermediate data structure - a vector, each entry containing matching `Div, Rem` pair, and pre-filling it before doing any modifications. This way we won't need to query map after doing RAUW, so no bug is possible. Reviewers: spatel, bogner, RKSimon, craig.topper Reviewed By: spatel Subscribers: hiraditya, hans, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D65451 llvm-svn: 367417 2019-07-31T12:06:16+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-31T12:06:16+00:00 urn:sha1:0d6048073795cc686552f228bdb11b2912dcef65 llvm-svn: 367415 2019-07-30T15:46:03+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-30T15:46:03+00:00 urn:sha1:5e0adce40f3481246c887ccfe4bb67573539e5de The @srem_of_srem_expanded case exposed a RAUW pitfall in D65298. Right now these don't appear to fail verification, so it should be safe to precommit them. https://reviews.llvm.org/D65298 https://bugs.llvm.org/show_bug.cgi?id=42823 https://reviews.llvm.org/D65451 llvm-svn: 367325 2019-07-30T07:44:58+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-30T07:44:58+00:00 urn:sha1:8e0cf076aca8e9c23919d1c8398826ab91a88560 test-suite/MultiSource/Benchmarks/DOE-ProxyApps-C/miniGMG broke: Only PHI nodes may reference their own value! %sub33 = srem i32 %sub33, %ranks_in_i This reverts commit r367288. llvm-svn: 367289 2019-07-30T07:10:00+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-30T07:10:00+00:00 urn:sha1:c75cdd056f6993bfed0962845e2f79f062a4bc11 Summary: While `-div-rem-pairs` pass can decompose rem in div+rem pair when div-rem pair is unsupported by target, nothing performs the opposite fold. We can't do that in InstCombine or DAGCombine since neither of those has access to TTI. So it makes most sense to teach `-div-rem-pairs` about it. If we matched rem in expanded form, we know we will be able to place div-rem pair next to each other so we won't regress the situation. Also, we shouldn't decompose rem if we matched already-decomposed form. This is surprisingly straight-forward otherwise. https://bugs.llvm.org/show_bug.cgi?id=42673 Reviewers: spatel, RKSimon, efriedma, ZaMaZaN4iK, bogner Reviewed By: bogner Subscribers: bogner, hiraditya, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D65298 llvm-svn: 367288 2019-07-25T20:26:34+00:00 Roman Lebedev lebedev.ri@gmail.com 2019-07-25T20:26:34+00:00 urn:sha1:aa205957ff6b977978642544951d133d47294d92 As discussed in https://bugs.llvm.org/show_bug.cgi?id=42673 there is a TTI hook hasDivRemOp() that matters here. While -div-rem-pairs will decompose 'rem' if that hook returns false, nothing does the opposite transform. We can't to this in InstCombine, because it does not currently access TTI, and i'm not sure we should change that. We can't really do that in DAGCombine since it also currently does not access TTI. Therefore only DivRemPairs is left. https://bugs.llvm.org/show_bug.cgi?id=42673 llvm-svn: 367046 2019-06-17T09:51:07+00:00 Fangrui Song maskray@google.com 2019-06-17T09:51:07+00:00 urn:sha1:ac14f7b10cffe2be548607269e036244cd16acc3 llvm-svn: 363538 2019-04-17T04:52:47+00:00 Eric Christopher echristo@gmail.com 2019-04-17T04:52:47+00:00 urn:sha1:cee313d288a4faf0355d76fb6e0e927e211d08a5 The reversion apparently deleted the test/Transforms directory. Will be re-reverting again. llvm-svn: 358552