[PATCH] D87489: [NFC][MLInliner] Presort instruction successions.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:280-286
 InlineSizeEstimatorAnalysis::InlineSizeEstimatorAnalysis() {}
 InlineSizeEstimatorAnalysis ::InlineSizeEstimatorAnalysis(
     InlineSizeEstimatorAnalysis &&) {}
 InlineSizeEstimatorAnalysis::~InlineSizeEstimatorAnalysis() {}
 InlineSizeEstimatorAnalysis::Result
 InlineSizeEstimatorAnalysis::run(const Function &F,
                                  FunctionAnalysisManager &FAM) {
----------------
dblaikie wrote:
> Hmm - I know when went around a few times on the API a while ago - and perhaps there's good reasons I've since forgotten (sorry :/) but this sort of if/else is something I was hoping to avoid. (by, eg: Having a virtual interface, a TF implementation and a non-TF stub implementation (or fallbacks in the calling code that tested for the absence of an implementation - "if null do the stub/default thing"), and then one function with the #if/else that - and then a single always-compiled function that has a conditional #include and then a "get estimator" function that returns null/default, or the TF implementation if available) - having lots of surface area like this means more chance of failures when someone's building only in one mode or the other. Specifically having different implementations of the same function I think is something best avoided where possible (LLVM has some in the platform APIs, for instance - open a file for Windows, open a file for Linux, etc - but in this case I hope it could be simpler by having only one very small amount of preprocessor conditionalized implementation)
We did that for the big things - the inline advisor. Here, there's not much of a surface area, and seemed more pragmatic to do it this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87489