[PATCH] D155789: [Support] Implement LLVM_ENABLE_REVERSE_ITERATION for StringMap

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/Support/StringMap.cpp:89-90
   unsigned FullHashValue = xxHash64(Name);
+  if (shouldReverseIterate())
+    FullHashValue = ~FullHashValue;
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);
----------------
dblaikie wrote:
> Does this actually cause reverse iteration? I'd guess it causes some other iteration, but not necessarily reverse? Maybe doesn't really matter, or maybe it'd be good to rename the reverse iteration thing?
> 
> (or maybe we could/should move to something more like Abseil that randomizes hash seeds/iteration order more aggressively/broadly, and doesn't require opt-in?)
It's not.


================
Comment at: llvm/lib/Support/StringMap.cpp:89-90
   unsigned FullHashValue = xxHash64(Name);
+  if (shouldReverseIterate())
+    FullHashValue = ~FullHashValue;
   unsigned BucketNo = FullHashValue & (NumBuckets - 1);
----------------
MaskRay wrote:
> dblaikie wrote:
> > Does this actually cause reverse iteration? I'd guess it causes some other iteration, but not necessarily reverse? Maybe doesn't really matter, or maybe it'd be good to rename the reverse iteration thing?
> > 
> > (or maybe we could/should move to something more like Abseil that randomizes hash seeds/iteration order more aggressively/broadly, and doesn't require opt-in?)
> It's not.
It's approximate reverse iteration, so the commit message actually says:

... shuffle StringMap iteration order (actually flipping the hash so that elements not in the
same bucket are reversed) to catch violations,

If two elements are hashed to share the same low order bits, whether or not the hash is inverted, their iteration order will be decided by the insertion order (linear probing characteristic). This makes the approach worse than a full-blown approach that changes many places to do a reversed iteration. Personally I feel such a full-blown approach is probably going to make the code too complex...


================
Comment at: llvm/test/CodeGen/MIR/AMDGPU/virtreg-uses-unallocatable-class.mir:1
+# UNSUPPORTED: reverse_iteration
 # RUN: not llc -mtriple=amdgcn-- -mcpu=gfx900 -run-pass=none -o - %s 2>&1 | FileCheck %s
----------------
foad wrote:
> MaskRay wrote:
> > @arsenm perhaps you can help fix this issue :)
> `MIRParserImpl::setupRegisterInfo` iterates over the StringMap `PerFunctionMIParsingState::VRegInfosNamed`. This only seems to affect the order in which error messages are printed. We could fix the test with `CHECK-DAG:`.
Thank you for mentioning the fix. Filed https://github.com/llvm/llvm-project/issues/64085 

Ideally diagnostics have the same order... I assume that MIRParser will not become slower if we add a `llvm::sort`


================
Comment at: llvm/test/ExecutionEngine/RuntimeDyld/ARM/MachO_ARM_PIC_relocations.s:1
+# UNSUPPORTED: reverse_iteration
 # RUN: rm -rf %t && mkdir -p %t
----------------
dblaikie wrote:
> Got bugs/outstanding reviews tracking these issues?
Filed https://github.com/llvm/llvm-project/issues/64085  Only 2 tests need to be fixed :)


================
Comment at: llvm/utils/lit/lit/llvm/config.py:123-125
+        if getattr(config, "reverse_iteration", None):
+            features.add("reverse_iteration")
+
----------------
dblaikie wrote:
> Do you plan to remove this feature in a fairly short time frame? Might be good to have a bug or review identifier here to track cleaning this up?
Replied on the other comment. Yes, this is hopefully short-term.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155789