[PATCH] D82817: [llvm] Native size estimator for training -Oz inliner

Tue Jul 14 19:40:13 PDT 2020

mtrofin added a comment.

In D82817#2151680 <https://reviews.llvm.org/D82817#2151680>, @mehdi_amini wrote:

> In D82817#2151520 <https://reviews.llvm.org/D82817#2151520>, @mtrofin wrote:
>
> > In D82817#2151481 <https://reviews.llvm.org/D82817#2151481>, @mehdi_amini wrote:
> >
> > > I have some concerns about TensorFlow types and datastructure being directly exposed to Analysis, and relying on macros to conditionally compile all this away.
> > >  This seems to me like an indication of a missing layering, and some dependencies to cut through injection somehow.
> > >
> > > Can you explain what in InlineSizeEstimatorAnalysis.cpp depends on TF for example? That would be a good starting point to help me understand how to break this dependency.
> > >
> > > Thanks!
> >
> >
> > In 'development' mode, InlineSizeEstimatorAnaysis uses a tensorflow model to estimate the native size of a function from some features.
> >  In this upcoming patch (https://reviews.llvm.org/D83733), also in 'development' mode, we use a tensorflow model that we want to load via command line (see the patch description).
> >
> > Note that the TF APIs are those limited to the C API surface area - basic data types and TF_Tensor. If one builds LLVM with TF API support, and then wants to add an analysis, they may benefit from reusing this API.
>
>
> I understand all this, I am still not sure we can't abstract the TF API away. I can clarify my earlier question: can you point me exactly what TF types and APIs needs to be exposed? To understand this I think that starting by `InlineSizeEstimatorAnaysis` may be a good idea, but skimming through it I only see the `FunctionFeatures` being expressed in terms of `std::vector` of various integer sizes: it isn't clear to me what from TensorFlow is used in this file right now?

https://reviews.llvm.org/D83843 should address this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82817