[PATCH] D89447: [MachineInstr] Add support for instructions with multiple memory operands.

Mon Oct 26 07:14:19 PDT 2020

hliao marked an inline comment as done.
hliao added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1733
+  /// in MachineInstr returns conservative result to avoid quadratic overhead.
+  virtual unsigned getMemOperandAACheckLimit() const { return 4; }
+
----------------
dmgreen wrote:
> So the limit on the number of alias checks is effectively 16?
> 
> Would it be worth making it so limit is on the total number of checks, not the MemOperands per instruction? That way an instruction with a single operand could be compared to an instruction with many (which I imagine would be a common case).
Just IMHO, the limit here is more straightforward to figure out. The backend needs to consider the corner case where two instructions with more than one mem operands are checked.

================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:1292
+    // Assume `MayAlias` if any memory operand is not unordered one.
+    if (!MMOa->isUnordered() || !MMOb->isUnordered())
       return true;
----------------
dmgreen wrote:
> Why is this new ordered check needed? I didn't think that atomics had multiple memory operands.
I added that just as future proof just like the load/store check. Just remove them to keep this change as minimal as possible.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89447