[PATCH] D127095: [llvm][CodeGen] Add a default implementation to check whether two memory accesses are trivially disjoint

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1842
+  /// Return true if two memory address spaces may overlap and false otherwise.
+  virtual bool mayAddressSpacesOverlap(unsigned AddrSpaceA,
+                                       unsigned AddrSpaceB) const {
----------------
Please don't add "virtual" methods to MachineInstr; it doesn't have any derived classes.

If you need some target-specific info, look at TargetLoweringInfo.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1856
+                                  uint64_t BaseAddrB, uint64_t OffsetB,
+                                  uint64_t WidthB, const Align &AlignB) const;
+
----------------
Does areMemAccessesTriviallyDisjoint really need to be in the header?

"Align" is just an integer; don't pass it by const reference.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2352
+    if (CE->getOpcode() == Instruction::IntToPtr &&
+        isa<ConstantInt>(CE->getOperand(0))) {
+      auto Op0 = dyn_cast<Constant>(CE->getOperand(0));
----------------
Probably want to check that the operand of the IntToPtr isn't wider than the pointer type.

Also, getZExtValue() will assert on targets that support pointer widths greater than 64.


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2367
+  // Check whether two memory accesses overlap, ignoring alignment.
+  const auto StartAddrA = BaseAddrA + OffsetA;
+  const auto EndAddrA = StartAddrA + WidthA;
----------------
Don't think "auto" makes sense here; see https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable .


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:2377
+  if (!mayAddressSpacesOverlap(MMOa->getAddrSpace(), MMOb->getAddrSpace())) {
+    return true;
+  }
----------------
I think this is missing a check for whether the address-spaces are different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127095