<div dir="ltr">Oh, cool, thanks! </div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 3, 2015 at 10:50 AM, Jonathan Roelofs <span dir="ltr"><<a href="mailto:jroelofs.lists@gmail.com" target="_blank">jroelofs.lists@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 9/3/15 11:48 AM, Kostya Serebryany via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Any suggestion?<br>
</blockquote>
<br>
d0k fixed this in r246694.<br>
<br>
<br>
Cheers,<br>
<br>
Jon<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<br>
On Wed, Sep 2, 2015 at 11:09 AM, Jonathan Roelofs via llvm-commits<br></span><div><div class="h5">
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>> wrote:<br>
<br>
<br>
<br>
On 9/2/15 12:07 PM, Mail Delivery System wrote:<br>
<br>
Forwarding to the new list...<br>
<br>
<br>
<br>
On 6/18/15 10:01 AM, Benjamin Kramer wrote:<br>
<br>
Author: d0k<br>
Date: Thu Jun 18 11:01:00 2015<br>
New Revision: 240023<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=240023&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=240023&view=rev</a><br>
Log:<br>
[EliminateDuplicatePHINodes] Replace custom hash map with<br>
DenseSet.<br>
<br>
While there use hash_combine instead of hand-rolled hashing. No<br>
functionality change intended.<br>
<br>
<br>
Something about this patch is causing a crash in the attached<br>
testcase<br>
(which is already heavily reduced/redacted as much as I could). The<br>
failure mode is an assertion fail in DenseMap:<br>
<br>
Assertion failed: (!FoundVal && "Key already in new map?"), function<br>
moveFromOldBuckets, file<br>
/Users/jroelofs/workdir/svn/llvm2/include/llvm/ADT/DenseMap.h,<br>
line 301.<br>
<br>
Unfortunately I can't get this to reproduce on trunk, and my<br>
stabs at<br>
debugging it have been thus-far unsuccessful... Mind taking a look?<br>
<br>
<br>
Thanks,<br>
<br>
Jon<br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Utils/Local.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Utils/Local.cpp<br>
URL:<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/Local.cpp?rev=240023&r1=240022&r2=240023&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/Local.cpp?rev=240023&r1=240022&r2=240023&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Utils/Local.cpp Thu Jun 18<br>
11:01:00 2015<br>
@@ -14,6 +14,8 @@<br>
<br>
#include "llvm/Transforms/Utils/Local.h"<br>
#include "llvm/ADT/DenseMap.h"<br>
+#include "llvm/ADT/DenseSet.h"<br>
+#include "llvm/ADT/Hashing.h"<br>
#include "llvm/ADT/STLExtras.h"<br>
#include "llvm/ADT/SmallPtrSet.h"<br>
#include "llvm/ADT/Statistic.h"<br>
@@ -828,64 +830,45 @@ bool llvm::TryToSimplifyUncondBranchFrom<br>
/// orders them so it usually won't matter.<br>
///<br>
bool llvm::EliminateDuplicatePHINodes(BasicBlock *BB) {<br>
- bool Changed = false;<br>
-<br>
// This implementation doesn't currently consider<br>
undef operands<br>
// specially. Theoretically, two phis which are<br>
identical except for<br>
// one having an undef where the other doesn't could<br>
be collapsed.<br>
<br>
- // Map from PHI hash values to PHI nodes. If multiple<br>
PHIs have<br>
- // the same hash value, the element is the first PHI in the<br>
- // linked list in CollisionMap.<br>
- DenseMap<uintptr_t, PHINode *> HashMap;<br>
+ struct PHIDenseMapInfo {<br>
+ static PHINode *getEmptyKey() {<br>
+ return DenseMapInfo<PHINode *>::getEmptyKey();<br>
+ }<br>
+ static PHINode *getTombstoneKey() {<br>
+ return DenseMapInfo<PHINode *>::getTombstoneKey();<br>
+ }<br>
+ static unsigned getHashValue(PHINode *PN) {<br>
+ // Compute a hash value on the operands. Instcombine<br>
will likely have<br>
+ // sorted them, which helps expose duplicates, but we<br>
have to check all<br>
+ // the operands to be safe in case instcombine hasn't<br>
run.<br>
+ return static_cast<unsigned>(hash_combine(<br>
+ hash_combine_range(PN->value_op_begin(),<br>
PN->value_op_end()),<br>
+ hash_combine_range(PN->block_begin(),<br>
PN->block_end())));<br>
+ }<br>
+ static bool isEqual(PHINode *LHS, PHINode *RHS) {<br>
+ if (LHS == getEmptyKey() || LHS == getTombstoneKey() ||<br>
+ RHS == getEmptyKey() || RHS == getTombstoneKey())<br>
+ return LHS == RHS;<br>
+ return LHS->isIdenticalTo(RHS);<br>
+ }<br>
+ };<br>
<br>
- // Maintain linked lists of PHI nodes with common hash<br>
values.<br>
- DenseMap<PHINode *, PHINode *> CollisionMap;<br>
+ // Set of unique PHINodes.<br>
+ DenseSet<PHINode *, PHIDenseMapInfo> PHISet;<br>
<br>
// Examine each PHI.<br>
- for (BasicBlock::iterator I = BB->begin();<br>
- PHINode *PN = dyn_cast<PHINode>(I++); ) {<br>
- // Compute a hash value on the operands. Instcombine<br>
will likely have sorted<br>
- // them, which helps expose duplicates, but we have to<br>
check all the<br>
- // operands to be safe in case instcombine hasn't run.<br>
- uintptr_t Hash = 0;<br>
- // This hash algorithm is quite weak as hash functions<br>
go, but it seems<br>
- // to do a good enough job for this particular purpose,<br>
and is very quick.<br>
- for (User::op_iterator I = PN->op_begin(), E =<br>
PN->op_end(); I != E; ++I) {<br>
- Hash ^= reinterpret_cast<uintptr_t>(static_cast<Value<br>
*>(*I));<br>
- Hash = (Hash << 7) | (Hash >> (sizeof(uintptr_t) *<br>
CHAR_BIT - 7));<br>
- }<br>
- for (PHINode::block_iterator I = PN->block_begin(), E =<br>
PN->block_end();<br>
- I != E; ++I) {<br>
- Hash ^=<br>
reinterpret_cast<uintptr_t>(static_cast<BasicBlock *>(*I));<br>
- Hash = (Hash << 7) | (Hash >> (sizeof(uintptr_t) *<br>
CHAR_BIT - 7));<br>
- }<br>
- // Avoid colliding with the DenseMap sentinels ~0 and ~0-1.<br>
- Hash >>= 1;<br>
- // If we've never seen this hash value before, it's a<br>
unique PHI.<br>
- std::pair<DenseMap<uintptr_t, PHINode *>::iterator,<br>
bool> Pair =<br>
- HashMap.insert(std::make_pair(Hash, PN));<br>
- if (Pair.second) continue;<br>
- // Otherwise it's either a duplicate or a hash collision.<br>
- for (PHINode *OtherPN = Pair.first->second; ; ) {<br>
- if (OtherPN->isIdenticalTo(PN)) {<br>
- // A duplicate. Replace this PHI with its duplicate.<br>
- PN->replaceAllUsesWith(OtherPN);<br>
- PN->eraseFromParent();<br>
- Changed = true;<br>
- break;<br>
- }<br>
- // A non-duplicate hash collision.<br>
- DenseMap<PHINode *, PHINode *>::iterator I =<br>
CollisionMap.find(OtherPN);<br>
- if (I == CollisionMap.end()) {<br>
- // Set this PHI to be the head of the linked list<br>
of colliding PHIs.<br>
- PHINode *Old = Pair.first->second;<br>
- Pair.first->second = PN;<br>
- CollisionMap[PN] = Old;<br>
- break;<br>
- }<br>
- // Proceed to the next PHI in the list.<br>
- OtherPN = I->second;<br>
+ bool Changed = false;<br>
+ for (auto I = BB->begin(); PHINode *PN =<br>
dyn_cast<PHINode>(I++);) {<br>
+ auto Inserted = PHISet.insert(PN);<br>
+ if (!Inserted.second) {<br>
+ // A duplicate. Replace this PHI with its duplicate.<br>
+ PN->replaceAllUsesWith(*Inserted.first);<br>
+ PN->eraseFromParent();<br>
+ Changed = true;<br>
}<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br></div></div>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>><span class=""><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
<br>
--<br>
Jon Roelofs<br></span>
<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a> <mailto:<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a>><span class=""><br>
CodeSourcery / Mentor Embedded<br>
_______________________________________________<br>
llvm-commits mailing list<br></span>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><span class=""><br>
<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</span></blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
Jon Roelofs<br>
<a href="mailto:jonathan@codesourcery.com" target="_blank">jonathan@codesourcery.com</a><br>
CodeSourcery / Mentor Embedded<br>
</div></div></blockquote></div><br></div>