[AMDGPU] - Fix non-deterministic compile issue

[dstutt]

4ce1f9079d4d3 [AMDGPU] Allow rematerialization of instructions with virtual register uses (#124327)
made changes that require an ordered traversal of a DenseMap. Changing it to MapVector which
respects insertion order.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: David Stuttard (dstutt)

Changes

4ce1f9079d4d3 [AMDGPU] Allow rematerialization of instructions with virtual register uses (#124327)
made changes that require an ordered traversal of a DenseMap. Changing it to MapVector which
respects insertion order.

---

Full diff: https://github.com/llvm/llvm-project/pull/126271.diff

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/GCNSchedStrategy.h (+1-1)

diff --git a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.h b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.h
index 7d3e63df43da60..e3da8d30056293 100644
--- a/llvm/lib/Target/AMDGPU/GCNSchedStrategy.h
+++ b/llvm/lib/Target/AMDGPU/GCNSchedStrategy.h
@@ -442,7 +442,7 @@ class PreRARematStage : public GCNSchedStage {
 
   // Map a trivially rematerializable def to a list of regions at MinOccupancy
   // that has the defined reg as a live-in.
-  DenseMap<MachineInstr *, SmallVector<unsigned, 4>> RematDefToLiveInRegions;
+  MapVector<MachineInstr *, SmallVector<unsigned, 4>> RematDefToLiveInRegions;
 
   // Collect all trivially rematerializable VGPR instructions with a single def
   // and single use outside the defining block into RematerializableInsts.

[jayfoad]

Sounds reasonable, but I don't immediately see anything in #124327 which iterates over RematDefToLiveInRegions. How and where does that happen?

[dstutt]

Sounds reasonable, but I don't immediately see anything in #124327 which iterates over RematDefToLiveInRegions. How and where does that happen?

There's an iteration over RematDefToLiveInRegions in sinkTriviallyRematInsts - which pre-dates the changes in #124327
My assumption is that the change enabled more rematerialization - and that's highlighted this pre-existing issue that probably didn't trigger very often before.
Note that we're only seeing the issue on 2 test pipelines out of 10k+ and only for one asic variant.

The resulting code is going to be functionally correct in both cases, so this only gets picked up when we specifically check for non-determinism.

[jrbyrnes]

There's an iteration over RematDefToLiveInRegions in sinkTriviallyRematInsts - which pre-dates the changes in #124327
My assumption is that the change enabled more rematerialization - and that's highlighted this pre-existing issue that probably didn't trigger very often before.

This is correct -- thanks