[PATCH] D15230: [BranchFolding] Merge MMOs during tail merge

[Junmo Park via llvm-commits]

flyingforyou added inline comments.

================
Comment at: include/llvm/CodeGen/MachineInstr.h:350
@@ -349,2 +349,3 @@
   bool memoperands_empty() const { return NumMemRefs == 0; }
+  uint8_t getNumMemOperands() const { return NumMemRefs; }
 
----------------
gberry wrote:
> I don't think you want to have the uint8_t implementation detail leak out of the interface here.  Perhaps this should be 'unsigned'?
> I'd like to hear from @hfinkel on this detail before I can approve it.
Thanks Geoff. Before waiting review from @hfinkel, I want to talk something.

We choose the way to check threshold MI1 MMO's count * MI2's MMO's count. If we change uint8_t to unsigned, it seems that can return over 255. This means we may take care overflow. Because the type of merge-mmos-threshold is unsigned. it's not uint64_t.

If @hfinkel also think change the return type to unsigned, we need to add assert before multipication.

And MachineInstr has two more uint8_t variables Flags, AsmPrinterFlags. Their getter & setter also have same return type. 
So I think this function may be ok also.

Junmo.


http://reviews.llvm.org/D15230