[RemoveDIs] Account for DPVAssigns in isIdenticalToWhenDefined

[OCHyams]

AddressExpression wasn't included in the comparison.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

AddressExpression wasn't included in the comparison.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+4-5)

diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index a983280d2c4b9a..594f613aa8ed8b 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -271,16 +271,15 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   std::optional<uint64_t> getFragmentSizeInBits() const;
 
   bool isEquivalentTo(const DPValue &Other) {
-    return std::tie(Type, DebugValues, Variable, Expression, DbgLoc) ==
-           std::tie(Other.Type, Other.DebugValues, Other.Variable,
-                    Other.Expression, Other.DbgLoc);
+    return DbgLoc == Other.DbgLoc && isIdenticalToWhenDefined(Other);
   }
   // Matches the definition of the Instruction version, equivalent to above but
   // without checking DbgLoc.
   bool isIdenticalToWhenDefined(const DPValue &Other) {
-    return std::tie(Type, DebugValues, Variable, Expression) ==
+    return std::tie(Type, DebugValues, Variable, Expression,
+                    AddressExpression) ==
            std::tie(Other.Type, Other.DebugValues, Other.Variable,
-                    Other.Expression);
+                    Other.Expression, Other.AddressExpression);
   }
 
   /// @name DbgAssign Methods

[jmorse]

LGTM (after reminding myself that the "WhenDefined" method is only there to match the Instruction version, it doesn't really mean it interacts with Undef/Poison).

[rupprecht]

It looks like this commit caused some non-determinism which is fixed recently by #83242. Is that known/expected?
I've so far only narrowed it down to building w/ the combination of optimized and split dwarf. The bits in the .o file are different, but I haven't inspected how they are different.

[OCHyams]

Thanks @rupprecht

Is that known/expected?

It wasn't, but having a look around I think I understand why it started here and has been since fixed.

This patch added AddressExpression to a comparison. AddressExpression isn't always used by this class (it's only used when the "kind" is LocationType::Assign), and it looks like one of the ctors for the class doesn't initialize it.

#83242 changes AddressExpression to a new type which wraps a pointer that is member initialized to nullptr.

So I imagine the uninitialized garbage in AddressExpression when that one ctor was used was throwing off the comparision, and it was stealth fixed because AddressExpression is now always initialized properly.