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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/ADT/SmallPtrSet.h:290
+#endif
     return *this;
   }
----------------
I'd write this:

```
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
    if (ReverseIterate<bool>::value) {
      RetreatIfNotValid();
      return *this;
    }
#endif
    ++Bucket;
    AdvanceIfNotValid();
   return *this;
```


================
Comment at: include/llvm/ADT/SmallPtrSet.h:301
+    ++*this;
+    return tmp;
   }
----------------
Similar as a above, that makes the `LLVM_ENABLE_ABI_BREAKING_CHECKS` path more self-contained.


================
Comment at: unittests/ADT/CMakeLists.txt:67
+if(DEFINED LLVM_ENABLE_ABI_BREAKING_CHECKS)
+  add_definitions(-DLLVM_ENABLE_ABI_BREAKING_CHECKS)
+  list(APPEND ADTSources ReverseIterationTest.cpp)
----------------
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.


https://reviews.llvm.org/D26718