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

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.

lg



================
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;
+  }
----------------
chandlerc wrote:
> 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?
The new comments are much clearer. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D50701