[PATCH] D50701: [MI] Change the array of `MachineMemOperand` pointers to be a generically extensible collection of extra info attached to a `MachineInstr`.

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 15 17:36:13 PDT 2018


chandlerc added a comment.

In https://reviews.llvm.org/D50701#1201714, @rnk wrote:

> I'm happy with this, but I assume you want to respond to @bogner's comment.


Thanks, and yeah. See below where I've tried to respond.



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:410-415
+  // We can never merge with an empty list as that must be treated
+  // conservatively.
+  if (MIs[0]->memoperands_empty()) {
+    dropMemRefs(MF);
+    return;
+  }
----------------
bogner wrote:
> This comment is overly vague. Why does this need to be treated conservatively? 
> 
> IIUC from the old comments this was because we could hit the case where we ran out of space and dropped memrefs, which meant that empty memrefs may have already hit that case and merging would give a subset. However, if this is still the case I don't see where handle the case of having too many.
> 
> Is there another case that this is trying to handle?
While the old code *also* had to handle running out of space for the operands, an empty list still can't be merged.

The issue is that the memoperands constrain what possible memory is accessed. An empty list is the *least* constrained, and so there is no way to merge with it. See the comments on the memoperands methods themselves. We're required to assume that in the absence of precise memoperands the instruction can access any memory it wants.

I've tried to clarify the comments here, but not sure it's really working. Feel free to suggest better comments here? Or let me know if this still isn't clear?


Repository:
  rL LLVM

https://reviews.llvm.org/D50701





More information about the llvm-commits mailing list