<div dir="ltr">No, this seems like a good reason to use two. See my prior email which mentioned it.<br><br><div>In my first email I just *asked* if there was a good reason. ;]</div></div><br><div class="gmail_quote">On Mon, Apr 6, 2015 at 3:57 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">So, i can't find a testcase up to 100k bb's where *anything* matters<br>
in terms of a vs b.<br>
I also tried it on 50k functions of 1000 bb's as well.<br>
<br>
In trying to use setvector, i found another set of much simpler<br>
reasons it won't work:<br>
<br>
1. We clear worklist every time we pop from the PQ. We can't do this<br>
with setvector<br>
2. We pop values off the worklist, which would erase them from<br>
setvector, but as implemented, we don't want them revisited anyway.<br>
<br>
IE Right now,  a value inserted into visitedworklist stays there the<br>
entire function, but a value inserted into worklist is cleared every<br>
time we pop something off the PQ.<br>
<br>
<br>
#1 is pointless (in the sense that the worklist is always empty at the<br>
point it's called, i assume it's there to do shrink to fit or<br>
something)<br>
<br>
Unless #2 is a bug, we can't use setvector.<br>
<br>
<br>
<br>
<br>
On Mon, Apr 6, 2015 at 3:17 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br>
> I'll time it.<br>
><br>
> I'll use a GCC testcase that generates fully connected graphs.<br>
><br>
> (I've already tried 10000+ bb cases with if's and and merges, and<br>
> can't get a vs b to have any real time difference)<br>
><br>
><br>
> On Mon, Apr 6, 2015 at 2:04 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:<br>
>> Sorry, I clearly meant SmallSetVector (which uses SmallVector and<br>
>> SmallPtrSet).<br>
>><br>
>> Also, I was not supposing you would touch the blocks thing at all, nor<br>
>> thinking this would remove any non-determinism. It was just that if you're<br>
>> pushing onto a vector only when insertion into a set vector succeeds, it<br>
>> might make sense. There are still plenty of cases where it doesn't, either<br>
>> due to type mis-matches, or because you want to keep growing the set while<br>
>> shrinking and re-using space in the vector.<br>
>><br>
>> On Mon, Apr 6, 2015 at 2:02 PM Cameron Zwarich <<a href="mailto:zwarich@apple.com" target="_blank">zwarich@apple.com</a>> wrote:<br>
>>><br>
>>> Yeah, that’s a bigger problem. My original complaints 2) and 3) are<br>
>>> invalid because those are template parameters that could presumedly be<br>
>>> swapped out for SmallVector / SmallPtrSet anyways.<br>
>>><br>
>>> Cameron<br>
>>><br>
>>> > On Apr 6, 2015, at 2:00 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br>
>>> ><br>
>>> > Unless i'm missing something, you can't use setvector anyway, because<br>
>>> > it won't remove the non-determinism the sort is there to fix.<br>
>>> ><br>
>>> > That non-determinism comes from the fact that defblocks is a<br>
>>> > smallptrset, and even if the priority queue's order is stable, the<br>
>>> > only guarantee you get is that things at the same dom level end up<br>
>>> > around the same place in the priority queue (but not in a specific<br>
>>> > order).<br>
>>> ><br>
>>> ><br>
>>> > If domtreenodepair was domtreenodetriple (<BasicBlock, DomLevel,<br>
>>> > BBNumbers[BB]>), and sorted by second then third part of tuple,<br>
>>> > dfblocks could be a std::vector and you could use setvector, and not<br>
>>> > sort anything, because it would be deterministic.<br>
>>> ><br>
>>> > Whether any of this is worth it, no idea.<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > On Mon, Apr 6, 2015 at 1:56 PM, Cameron Zwarich <<a href="mailto:zwarich@apple.com" target="_blank">zwarich@apple.com</a>><br>
>>> > wrote:<br>
>>> >> I just thought it would make aesthetic sense to have less code in the<br>
>>> >> inner<br>
>>> >> loop, but that’s arbitrary. If people think the ‘a’ patch makes more<br>
>>> >> sense<br>
>>> >> and it’s equivalent perfwise, then I’m fine with that. Code is made for<br>
>>> >> reading, not writing. :P<br>
>>> >><br>
>>> >> I’m not sure SetVector makes sense here for a few reasons:<br>
>>> >><br>
>>> >> 1) The set and the vector store different types; the vector also<br>
>>> >> contains<br>
>>> >> the BB number.<br>
>>> >> 2) The vector in SetVector is a std::vector, not a SmallVector.<br>
>>> >> 3) The set in SetVector is a SmallSet of a fixed size of 16 (smaller<br>
>>> >> than<br>
>>> >> the 32 used here), and the ‘large’ case of SmallSet is a std::set,<br>
>>> >> which is<br>
>>> >> probably slower than SmallPtrSet’s ‘large’ case.<br>
>>> >><br>
>>> >> Cameron<br>
>>> >><br>
>>> >> On Apr 6, 2015, at 1:40 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>><br>
>>> >> wrote:<br>
>>> >><br>
>>> >> I'd like to understand why Cameron would prefer the 'b' patch to the<br>
>>> >> 'a'<br>
>>> >> patch. AFAICT, the 'b' patch doesn't save any actual memory (sadly).<br>
>>> >><br>
>>> >> Also, if we go with 'a', would it make sense to use a SetVector rather<br>
>>> >> than<br>
>>> >> a separate vector and set?<br>
>>> >><br>
>>> >> On Mon, Apr 6, 2015 at 1:36 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>><br>
>>> >> wrote:<br>
>>> >>><br>
>>> >>> I'm running numbers on both approaches now, to see if there is any<br>
>>> >>> real difference in speed.<br>
>>> >>><br>
>>> >>> (otherwise, i think the one with two visited worklists is easier to<br>
>>> >>> understand, unless someone else wants to disagree :P)<br>
>>> >>><br>
>>> >>><br>
>>> >>> On Mon, Apr 6, 2015 at 11:54 AM, Quentin Colombet<br>
>>> >>> <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>><br>
>>> >>> wrote:<br>
>>> >>>> Hi Cameron,<br>
>>> >>>><br>
>>> >>>> Sounds good.<br>
>>> >>>><br>
>>> >>>> Let us wait the complete testing from Daniel before reviewing the<br>
>>> >>>> patch.<br>
>>> >>>><br>
>>> >>>> Thanks,<br>
>>> >>>> -Quentin<br>
>>> >>>>> On Apr 6, 2015, at 11:27 AM, Cameron Zwarich <<a href="mailto:zwarich@apple.com" target="_blank">zwarich@apple.com</a>><br>
>>> >>>>> wrote:<br>
>>> >>>>><br>
>>> >>>>> It was pointed out to me (without any specifics) that the iterated<br>
>>> >>>>> dominance frontier algorithm in PromoteMemoryToRegister.cpp has<br>
>>> >>>>> O(n^2) worst<br>
>>> >>>>> case behavior.<br>
>>> >>>>><br>
>>> >>>>> I inspected the code and think I found the cause. The code uses a<br>
>>> >>>>> priority queue and a worklist, which share the same visited set, but<br>
>>> >>>>> the<br>
>>> >>>>> visited set is only updated when inserting into the priority queue.<br>
>>> >>>>> The<br>
>>> >>>>> original Sreedhar-Gao paper effectively has a second visited set<br>
>>> >>>>> (the InPhi<br>
>>> >>>>> flag) which is used for the priority queue, and the set called<br>
>>> >>>>> Visited is<br>
>>> >>>>> used for the recursive traversal that is done here with a worklist.<br>
>>> >>>>><br>
>>> >>>>> I’ve attached two patches, one which just adds a second visited sit,<br>
>>> >>>>> and another which leverages the fact that one of the visited sets is<br>
>>> >>>>> actually the IDF. I would prefer the latter if it has equal<br>
>>> >>>>> performance with<br>
>>> >>>>> the first.<br>
>>> >>>>><br>
>>> >>>>> They both pass `make check`, but I’m not sure I’ll have time to give<br>
>>> >>>>> these patches the testing they’ll deserve in the next few days.<br>
>>> >>>>> Daniel<br>
>>> >>>>> Berlin has offered to test them more thoroughly for me.<br>
>>> >>>>><br>
>>> >>>>> Note that there is still one difference with the paper. The paper<br>
>>> >>>>> uses<br>
>>> >>>>> a custom linked data structure instead of a priority queue, which<br>
>>> >>>>> takes<br>
>>> >>>>> advantage of the property that the level of all nodes being inserted<br>
>>> >>>>> is at<br>
>>> >>>>> most the current level. The code in LLVM uses a priority queue based<br>
>>> >>>>> on a<br>
>>> >>>>> binary heap. This means that the worst case is O(n log n), but I’d<br>
>>> >>>>> be<br>
>>> >>>>> surprised if the difference matters in practice.<br>
>>> >>>>><br>
>>> >>>>> Cameron<br>
>>> >>>>><br>
>>> >>>>><br>
>>> >>>>><br>
>>> >>>>> <idf-faster-a.patch><idf-<u></u>faster-b.patch>_______________<u></u>______________________________<u></u>__<br>
>>> >>>>> llvm-commits mailing list<br>
>>> >>>>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> >>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
>>> >>>><br>
>>> >>><br>
>>> >>> ______________________________<u></u>_________________<br>
>>> >>> llvm-commits mailing list<br>
>>> >>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
>>> >><br>
>>> >> ______________________________<u></u>_________________<br>
>>> >> llvm-commits mailing list<br>
>>> >> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
>>> >><br>
>>> >><br>
>>><br>
>><br>
</blockquote></div>