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

[Xiaoqiang Xu via Phabricator via llvm-commits]

csstormq 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 {
----------------
efriedma wrote:
> 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.
I moved these virtual functions into TargetMachine. Is this OK?


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1856
+                                  uint64_t BaseAddrB, uint64_t OffsetB,
+                                  uint64_t WidthB, const Align &AlignB) const;
+
----------------
efriedma wrote:
> Does areMemAccessesTriviallyDisjoint really need to be in the header?
> 
> "Align" is just an integer; don't pass it by const reference.
As the LLVM document says, the semantics of non-zero address spaces are target-specific. Thus the intent of areMemAccessesTriviallyDisjoint as virtual function is enable to cope with the odd behavior of some targets, although this funciton provides a default implementation for most cases.


================
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));
----------------
efriedma wrote:
> 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.
Excuse me, is there any problem if I don't check that?

I address the problem of getZExtValue soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127095