[PATCH] D26718: [llvm] Iterate SmallPtrSet in reverse order to uncover non-determinism in codegen

Mandeep Singh Grang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 13 13:09:23 PST 2016

mgrang marked an inline comment as done.
mgrang added inline comments.

Comment at: include/llvm/ADT/SmallPtrSet.h:290
     return *this;
mehdi_amini wrote:
> I'd write this:
> ```
>     if (ReverseIterate<bool>::value) {
>       RetreatIfNotValid();
>       return *this;
>     }
> #endif
>     ++Bucket;
>     AdvanceIfNotValid();
>    return *this;
> ```

Comment at: unittests/ADT/CMakeLists.txt:67
+  list(APPEND ADTSources ReverseIterationTest.cpp)
mehdi_amini wrote:
> mgrang wrote:
> > mehdi_amini wrote:
> > > mgrang wrote:
> > > > Without explicitly passing LLVM_ENABLE_ABI_BREAKING_CHECKS to the compilation line of ReverseIteration.cpp, it always complains about undefined variable "ReverseIterate".
> > > That's not great, but I don't see a better way right now, can you at least document it in file here?
> > Didn't realize we needed to include llvm/Config/abi-breaking.h in the header for LLVM_ABI_BREAKING_CHECKS to be used. After including this in the header we no longer need an explicit add_definitions.
> Even if you build in Release mode?
> My understanding was that the add_definitions was enabling the unittest all the time, regardless of the build configuration.
No, this test will only be enabled when LLVM_ENABLE_ABI_BREAKING_CHECKS is defined.

However, conditionally including this test file only when LLVM_ENABLE_ABI_BREAKING_CHECKS is defined produces an error in Release mode: "unknown file ReverseIteration.cpp"
So I guess we need to put the condition on the test instead of the file.


More information about the llvm-commits mailing list