[PATCH] D60982: [SimplifyCFG] Use lookup tables when they are more space efficient or a huge speed win.

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:582
+  /// this target.
+  bool shouldBuildLookupTable(size_t TableSize, uint32_t NumCases,
+                              unsigned CaseSize, bool HasOptSize) const;
----------------
shawnl wrote:
> jmolloy wrote:
> > To be honest, I'd really just express this as:
> > 
> >   // Return true if the given SwichInst should be converted into a lookup table. The size of the lookup table is \c TableSize and the number of covered cases is \c NumCases (meaning the number of table entries that hit the default case is \c TableSize - \c NumCases).
> >   bool shouldBuildLookupTable(SwitchInst *SI, uint64_t TableSize, unsigned NumCases);
> > 
> > Sorry for the churn here, I know I didn't mention this in your previous review. But passing the SwitchInst itself and letting the target fish out OptSize/SI->getType()->getIntegerBitWidth() seems cleaner.
> > 
> > Also, never use size_t or uint32_t here. TableSize's maximum extent isn't host dependent, it's target dependent. Use uint64_t to guarantee 64-bit coverage on all hosts. Similarly NumCases isn't limited to 32-bits, so use unsigned because that's what we use everywhere.
> good catch on the size_t!
> 
> > Similarly NumCases isn't limited to 32-bits, so use unsigned because that's what we use everywhere.
> 
> I was going to limit it to 32-bits in the next patch. Is that a problem?
I don't see the rationale for limiting it to 32-bits, so let's argue that in your next patch :)

In the meantime let's keep it to uint64_t or unsigned in this patch.


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

https://reviews.llvm.org/D60982