[PATCH] D64601: [MemorySSA] Use SetVector to avoid nondeterminism.

Fri Jul 12 12:38:21 PDT 2019

dblaikie added a comment.

In D64601#1583240 <https://reviews.llvm.org/D64601#1583240>, @asbirlea wrote:

> Hmm, AFAICT, there is no opt flag equivalent for LLVM_ENABLE_REVERSE_ITERATION. Did I miss it?

Nah, you didn't miss it - for performance reasons (I think/assume/vaguely recall) it's not a value that can be changed at runtime - it's a compile time configuration option.

Much like writing a test that fails (or, once some bug is fixed, passes) under asan - it's OK that a test might have only demonstrated the failure under one build mode or another, not necessarily under all of them. (heck, lots of tests are run against specific targets (which isn't always great, because sometimes the tests could be written portably and aren't (I've done this as much/more than anyone else - lots of the debug info tests are x86 only)) - which aren't compiled on every buildbot)

> Would adding Mikael's test with the following checks work? I'm thinking the use list order is pretty specific to add as a CHECK, hence the arch restriction, but it may still be too specific.
> 
>   ; RUN: opt -simplifycfg -enable-mssa-loop-dependency -S --preserve-ll-uselistorder %s | FileCheck %s
>   ; REQUIRES: x86-registered-target
>   ; CHECK-LABEL: @n
>   ; CHECK: uselistorder i16 0, { 3, 2, 4, 1, 5, 0, 6 }

If the test case is as reduced as possible while still demonstrating a change in behavior between forward and reverse iteration - yeah, I think that's OK. (not ideal, and maybe leave a note that the test case might not carry its weight - if someone in the future finds maintaining the test to be especially burdensome, it can probably be deleted without a huge deal)

Does this test case exercise/justify all the container changes in this patch? (seemed like a lot of containers got changed - which I'm a bit wary of because the non-ordered containers are more efficient, so best not to switch to them unnecessarily)

- Dave

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64601/new/

https://reviews.llvm.org/D64601