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

[Geoff Berry via llvm-commits]

gberry 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; }
 
----------------
flyingforyou wrote:
> 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.
Junmo, I agree with your points above, other than a small point that we shouldn't add an assert for overflow, but rather a check that handles it appropriately (i.e. by not doing the merge).

I think all that's left is for @hfinkel to chime in.


http://reviews.llvm.org/D15230