[llvm] [NFC] [DSE] Refactor DSE (PR #100956)

[Jan Voung via llvm-commits]

================
@@ -806,6 +806,38 @@ bool canSkipDef(MemoryDef *D, bool DefVisibleToCaller) {
   return false;
 }
 
+// A memory location wrapper that represents a MemoryLocation, `MemLoc`,
+// defined by `MemDef`.
+class MemoryLocationWrapper {
+public:
+  MemoryLocationWrapper(MemoryLocation MemLoc, MemoryDef *MemDef)
+      : MemLoc(MemLoc), MemDef(MemDef) {
+    assert(MemLoc.Ptr && "MemLoc should be not null");
+    UnderlyingObject = getUnderlyingObject(MemLoc.Ptr);
+    DefInst = MemDef->getMemoryInst();
+  }
+
+  MemoryAccess *GetDefiningAccess() const {
+    return MemDef->getDefiningAccess();
+  }
+
+  MemoryLocation MemLoc;
+  const Value *UnderlyingObject;
+  MemoryDef *MemDef;
+  Instruction *DefInst;
+};
+
+// A memory def wrapper that represents a MemoryDef and the MemoryLocation(s)
+// defined by this MemoryDef.
+class MemoryDefWrapper {
+public:
+  MemoryDefWrapper(MemoryDef *MemDef, std::optional<MemoryLocation> MemLoc) {
+    if (MemLoc.has_value())
+      DefinedLocation = MemoryLocationWrapper(*MemLoc, MemDef);
+  }
+  std::optional<MemoryLocationWrapper> DefinedLocation = std::nullopt;
----------------
jvoung wrote:

Hmm my preference is not to keep a reference to `MemoryDefWrapper` in each `MemoryLocationWrapper` and not make it circular =)

Sorry, maybe I should have discussed this more before asking about the comment updates, but I didn't notice this until later (already added note about comments).

What do you think of splitting out the MemDef as a separate parameter to `getDomMemoryDef` as it was before (KillingDef)? And then DefInst can be derived in a local scope as before (KillingI). Does DefInst need to be extracted early?

Can you clarify the benefits of bundling them in a single parameter?
* Initially you had eliminateDeadDefs as a method of the wrapper, but that's now moved back into DSEState.
* I can see that there's a lot of parameters for getDomMemoryDef and the relationships are loose and only tied together by the same name prefix "Killing"
* similar for the "DeadLoc", "DeadDefAccess",  ...

But then
* there is the repetition
* the "Wrapper" part of `MemoryLocationWrapper` name doesn't convey much information to the reader (related to the "killed by" comments where it was unclear there's a MemDef in the MemoryLocationWrapper class, and now say "killed by `KillingLocWrapper.MemDef"). Similar for the MemoryDefWrapper but the other way -- it may be unclear there are MemoryLocations in the class. If both Wrappers have access to the Def and some Locations it feels harder to tell the difference from the name.

A middle ground could be a single class "MemDefAndLocations" for bundling purposes (Killing vs Dead, etc.), but then allow splitting off MemDef and MemoryLocation when the execution starts focusing on one of the MemoryLocations related to the MemDef?


https://github.com/llvm/llvm-project/pull/100956