Page MenuHomePhabricator

[DAGCombiner] Unify Lifetime and memory Op aliasing.
ClosedPublic

Authored by niravd on Mar 25 2019, 12:28 PM.

Details

Summary

Rework BaseIndexOffset and isAlias to fully work with lifetime nodes
and fold in lifetime alias analysis.

This is mostly NFCI.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Mar 25 2019, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 12:28 PM
courbet added inline comments.Mar 26 2019, 12:51 AM
llvm/include/llvm/CodeGen/SelectionDAGAddressAnalysis.h
37 ↗(On Diff #192183)

What about using:

Optional<int64_t> Offset;

To avoid mistakes when handling IsOffsetValid ?

80 ↗(On Diff #192183)

s/BasePtr0/Op0/

86 ↗(On Diff #192183)

s/Ptr/N/

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19374 ↗(On Diff #192183)

why = 0 ? For consistency, it would be better to use it on all variables or none.

19378 ↗(On Diff #192183)

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.

19394 ↗(On Diff #192183)

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.

19402 ↗(On Diff #192183)

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);
19432 ↗(On Diff #192183)

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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
268 ↗(On Diff #192183)

I don't get the comment.

270 ↗(On Diff #192183)

please clang-format the patch.

niravd marked 8 inline comments as done.Mar 26 2019, 1:26 PM
niravd added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19432 ↗(On Diff #192183)

We shouldn't have to worry about semantically non-standard memory call like hardware control should be volatile already which precludes most alias improvements.

niravd updated this revision to Diff 192338.Mar 26 2019, 1:27 PM

Address comments.

niravd marked 2 inline comments as done.Mar 26 2019, 1:29 PM
niravd added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
19394 ↗(On Diff #192183)

I've wrapped it in an optional as we conservatively never use the size without the offset.

courbet accepted this revision.Mar 27 2019, 4:57 AM
courbet added inline comments.
llvm/include/llvm/CodeGen/SelectionDAGAddressAnalysis.h
36 ↗(On Diff #192338)

Maybe remove = 0 ?

This revision is now accepted and ready to land.Mar 27 2019, 4:57 AM
This revision was automatically updated to reflect the committed changes.
niravd marked an inline comment as done.