[PATCH] D59794: [DAGCombiner] Unify Lifetime and memory Op aliasing.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGAddressAnalysis.h:37
   int64_t Offset = 0;
+  bool IsOffsetValid = false;
   bool IsIndexSignExt = false;
----------------
What about using:
```
Optional<int64_t> Offset;
```

To avoid mistakes when handling IsOffsetValid ?


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGAddressAnalysis.h:80
 
   // Returns true `BasePtr0` and `BasePtr1` can be proven to alias/not alias, in
   // which case `IsAlias` is set to true/false.
----------------
s/BasePtr0/Op0/


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGAddressAnalysis.h:86
 
   /// Parses tree in Ptr for base, index, offset addresses.
+  static BaseIndexOffset match(const SDNode *N, const SelectionDAG &DAG);
----------------
s/Ptr/N/


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19374
+  int64_t Offset0, Offset1;
+  unsigned NumBytes0 = 0, NumBytes1 = 0;
+  MachineMemOperand *MMO0, *MMO1;
----------------
why `= 0` ? For consistency, it would be better to use it on all variables or none.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19378
+
+  auto getMemUseCharacteristics = [&](SDNode *N) {
+    if (const auto *LSN = dyn_cast<LSBaseSDNode>(N)) {
----------------
Please do not use implicit capture. It makes things harder to reason about. In particular, here, it hides the fact that there is actually no capture.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19394
+                             (LN->hasOffset()) ? LN->getOffset() : 0,
+                             false /*isVolatile*/, (LN->hasOffset()) ? (unsigned)LN->getSize() : (~0),
+                             (MachineMemOperand *)nullptr);
----------------
please add a comment to explain `~0`.
The original code had `// Be conservative if we don't know the extents of the object.`

Actually I'm not sure how the new code ensures that we never return `true` for `isAlias`, because the large size only means that we're saying that the lifetime aliases all memory **after** the base ptr, not before.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19402
 
-  // If they are both volatile then they cannot be reordered.
-  if (Op0->isVolatile() && Op1->isVolatile()) return true;
+  std::tie(BasePtr0, Offset0, IsVol0, NumBytes0, MMO0) =
+      getMemUseCharacteristics(Op0);
----------------
Easier to read in my opinion:

```
struct MemUseCharacteristics {
  SDValue BasePtr;
  int64_t Offset = 0;
  unsigned NumBytes = 0;
  MachineMemOperand* MMO = nullptr;
  bool IsVolatile = false;
};

auto getMemUseCharacteristics = [](SDNode *N) -> MemUseCharacteristics {
  ...
  if (const auto *LN = cast<LifetimeSDNode>(N))
      return {LN->getOperand(1),
                             LN->hasOffset() ? LN->getOffset() : 0,
                             LN->hasOffset() ? LN->getSize() : ~0,
                             nullptr, false /*isVolatile*/);
  return {};
}

const MemUseCharacteristics MUC0 = getMemUseCharacteristics(Op0);
const MemUseCharacteristics MUC1 = getMemUseCharacteristics(Op1);

```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19432
+
+  // If one operation reads from invariant memory, and the other may store, they
+  // cannot alias. These should really be checking the equivalent of mayWrite,
----------------
BTW: 
I'm not sure about this. For example, can we always reorder two writes of different values to invariant memory ? IIUC `isInvariant()` is about reading, but I could see some cases (e.g. hardware control) where invariant memory cares about write ordering.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:268
 
+// returns boolean if offset is meaningful.
+BaseIndexOffset BaseIndexOffset::match(const SDNode *N,
----------------
I don't get the comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:270
+BaseIndexOffset BaseIndexOffset::match(const SDNode *N,
+                             const SelectionDAG &DAG) {
+  if (const auto *LS0 = dyn_cast<LSBaseSDNode>(N))
----------------
please clang-format the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59794