[PATCH] D35043: [ADT] Enable reverse iteration for DenseMap

[David Blaikie via llvm-commits]

On Sun, Jul 23, 2017 at 5:24 PM Grang, Mandeep Singh <mgrang at codeaurora.org>
wrote:

> Thanks David and others for all your comments and suggestions so far.
>
> > I'm still not sure it's the best idea for this to be mutable - what
> would it be like if it were a compile time constant & no ifdefs were
> required for the use - and only the reverse build configuration would test
> that the reverse mode worked, etc.
>
> I am totally fine with making the reverse iteration variable a compile
> time constant which would be ON for reverse iteration builds and OFF by
> default. However, I have a couple of questions:
>
> 1. In the unit tests, how do we then check for the correct order of
> iteration?
>
I'd suggest hardcoding a specific order (based on observation) - this test
necessarily is testing implementation details anyway - it does make it a
little brittle, but some good comments about "update this list if the order
changes".

> Let's assume we have a reverse iteration build. We insert elements {&a,
> &b, &c, &d} into a container and when we iterate the container the elements
> are {&c, &d &a, &b}. Now how do we check whether this is the correct order
> (reverse order) or not. Whereas currently, in the unit test we manually
> turn reverse iteration ON/OFF so we know the default order and the reverse
> order.
>
> 2. If we make the variable a constant, we also need to get rid of the
> -mllvm -reverse-iteration flag.
>
Yep - I'm totally open to other opinions on this, maybe since it's already
a runtime flag it can stay that way.

- Dave

> This is not much of an issue except that we won't have a quick way to
> check things.
>
> --Mandeep
>
> On 7/15/2017 5:44 PM, David Blaikie wrote:
>
>
>
> On Wed, Jul 12, 2017 at 1:28 PM Mandeep Singh Grang via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> mgrang marked 2 inline comments as done.
>> mgrang added inline comments.
>>
>>
>> ================
>> Comment at: include/llvm/Support/ReverseIteration.h:26
>> +  return ReverseIterate<bool>::value &&
>> +         std::is_pointer<T>::value;
>>  }
>> ----------------
>> dblaikie wrote:
>> > Should this use an LLVM trait, to catch more interesting things that
>> are pointer-like but not is_pointer? (I guess unique/shared ptr, and maybe
>> PointerIntPair, PointerUnion, etc? Not sure if there's already an LLVM
>> trait that covers all of those)
>> Yes, ideally this should use an LLVM trait. I tried adding a "bool
>> IsPointerLikeType" field to each template specialization in
>> PointerLikeTypeTraits.h. As a result I had to add this field to each
>> definition/specialization of this template throughout llvm/clang/polly as
>> its definitions are scattered throughout.
>>
>> More pressingly, the compilation failed since it will now call
>> PointerLikeTypeTrait<T>::IsPointerLikeType for a T which does not have a
>> complete trait definition (like LLVMBasicBlockRef).
>>
>> I also tried adding a similar field to each specialization in
>> DenseMapInfo. But that would make it too specific as I would then need to
>> do something like "DenseMapInfo<T>::IsPointerLikeType". This would make it
>> difficult to generalize this for other containers.
>>
>> Could you please suggest some ways I can work around these?
>>
>
> Using only the existing PointerLikeTypeTrait, here's a type trait that
> might detect if something has a PointerLikeTypeTrait specialization:
>
> #include <utility>
> template <typename T> struct traits {};
>
> template <typename T> struct traits<T *> {
>   enum { foo = 3 };
> };
>
> template <typename T> struct is_pointer {
>   static char test_pointer(const void *);
>   template <typename T1>
>   static typename std::enable_if<traits<T1>::foo != 0, char (&)[2]>::type
>   test_pointer(T1 *);
>   static const bool value = sizeof(test_pointer(std::declval<T *>())) == 2;
> };
>
> static_assert(is_pointer<void *>::value, "");
> static_assert(!is_pointer<int>::value, "");
>
> (where 'foo' is 'NumLowBitsAvailable' (or you could make it work for teh
> other functions in PointerLikeTypeTraits - or even to check that all 3 are
> available before accepting that it's pointer like))
>
> Though maybe Richard Smith or others have better ideas about how to do
> this.
>
> - Dave
>
>
>>
>> ================
>> Comment at: unittests/ADT/ReverseIterationTest.cpp:30
>> +  // Check forward iteration.
>> +  ReverseIterate<bool>::value = false;
>> +  for (const auto &Tuple : zip(Map, Keys))
>> ----------------
>> dblaikie wrote:
>> > Having this as a mutable global variable seems surprising to me -
>> though maybe it is the right answer here for testing & the like, I'm not
>> sure.
>> >
>> > I'd be OK if it was a constant and this test tested the constant (yeah,
>> that'd mean there would be no coverage for reverse iteration in a build
>> that hadn't enabled reverse iteration, but that might be OK (I think of
>> reverse iteration-clean like being valgrind/sanitizer clean - yeah, we
>> might check in tests that are near-meaningless when run without sanitizers,
>> but they test that this scenario is sanitizer clean when the sanitizers are
>> enabled)) to decide which order to expect results in. Also maybe some
>> comments explaining that this is testing implementation details, clients
>> shouldn't depend on iteration order, and that these tests are here to
>> ensure that the intentional unreliability of the iteration order is
>> functioning.
>> Ideally, I want to make ReverseIteration a class and the "value" field a
>> private member and introduce setter/getter functions. I am currently
>> running through compilation failures with this. Will push a subsequent
>> patch once I get a clean build.
>>
>
> I'm still not sure it's the best idea for this to be mutable - what would
> it be like if it were a compile time constant & no ifdefs were required for
> the use - and only the reverse build configuration would test that the
> reverse mode worked, etc.
>
> - Dave
>
>
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D35043
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170724/5cfd8c12/attachment.html>