[PATCH] D71578: [CodeMoverUtils] Improve IsControlFlowEquivalent.

Whitney Tsang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 23 18:31:03 PST 2019


Whitney marked 4 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:101
+            !IsDifferent(Other.getFalseConditions(), FalseConditions,
+                         TrueConditions));
+  }
----------------
fhahn wrote:
> fhahn wrote:
> > Whitney wrote:
> > > Whitney wrote:
> > > > fhahn wrote:
> > > > > Whitney wrote:
> > > > > > jdoerfert wrote:
> > > > > > > What if you would only track positive conditions (negate the others) and you put them into a set. The is equivalent check will then be imple equality check on the sets. You can then also check "implication" through subset relation, and missing condition, through set difference.
> > > > > > I changed to use unordered_set. I created a ControlCondition struct instead as the type for the unordered_set. Do you think that's sufficient?
> > > > > This should use DenseMap, unless there is a good justification to not use it. Also ControlCondition could just be a tagged pointer (http://llvm.org/doxygen/classllvm_1_1PointerIntPair.html)
> > > > Do you feel strongly about changing to DenseMap? I tried to change it, but I could not make it work successfully. Somehow equivalent values are inserted to the same set. I can post my  code change as a comment and see if you could spot the problem, if you really want to change to a dense map. 
> > > The problem should be LookupBucketFor() find bucket of a value using its hash, so two Values isEquivalent() considered as equivalent could be in two different buckets, and both would be added to the set/map.
> > Are you sure there isn’t a problem with the hash function? 
> > 
> > After a quick look it seems like the hasher uses only the pointer of the value (and the int) for the hash value . So for example, two different icmp instructions will have different hashes.
> > 
> > But the comperator can consider those two different icmp instructions equal, if their condition matches and their operands are equivalent.
> > 
> > I don’t think that is allowed, equivalent values should always have the same hash (while it's fine for unequal elements to have the same hash). It probably works with unordered_map on the test cases by coincidence due to different default bucket sizes or something like that.
> > equivalent values 
> 
> it should probably say equal values according to the comparator.
Thanks for the info, didn't know `If k1 and k2 are equivalent, the hash function shall return the same value for both.`.  Changed to a SmallVector. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71578





More information about the llvm-commits mailing list