[PATCH] D77072: [mlir] LLVMFuncOp: provide a capability to pass attributes through to LLVM IR

[Alex Zinenko via Phabricator via llvm-commits]

ftynse marked an inline comment as done.
ftynse added inline comments.


================
Comment at: mlir/docs/Dialects/LLVM.md:107
+attribute.
+
 ### LLVM IR operations
----------------
mehdi_amini wrote:
> ftynse wrote:
> > ftynse wrote:
> > > mehdi_amini wrote:
> > > > nicolasvasilache wrote:
> > > > > mehdi_amini wrote:
> > > > > > That seems a bit like a hack to me: when importing from LLVM are we gonna encode attributes this way? Are we gonna honor them?
> > > > > > We're getting into the "json of compiler" here.
> > > > > > 
> > > > > Isn't the LLVM `string = string` attribute the issue here?
> > > > > Would an alternative require giving more semantics than LLVM has?
> > > > > 
> > > > > In other words, until/unless LLVM force more semantics on those attributes, isn't it a raising problem to import from LLVM and guesstimate types? 
> > > > LLVM has string attributes only for target specific I believe, otherwise there are rules for "IR-level" attributes: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Attributes.td 
> > > > (I am not sure I totally understood what you meant though, so my answer may be off here)
> > > I agree that it feels hacky, but so does LLVM's approach. You can add arbitrary quoted strings as function attributes, which some passes of various levels of experimental-ness rely on, and we got to support that. We have a need for that today to support some forms of vectorization. While we could restrict this feature to only support string attributes, we would need to encode a list of allowed attributes somewhere, which sounds equivalent to just building support for them, and we don't have time to invest in that right now.
> > > 
> > > I expect that we will eventually have first-class modeling for the equivalents of common attributes that may differ from what LLVM does with attributes (e.g., for inlining or FP math options). Until we do, we need to have a way to attach these attributes in the generated code for the purposes of code generation.
> > > 
> > > https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/Attributes.td is of little help because it does not say which attributes are allowed on functions (as opposed to, e.g., function attributes) and which of them are allowed to have values. Furthermore, there are attributes allowed on functions listed in https://llvm.org/docs/LangRef.html#function-attributes that are not listed in Attributes.td.
> > >  (as opposed to, e.g., function attributes)
> > 
> > I meant function _argument_ attributes
> I agree that LLVM is not perfect, but this is much uglier and I'm gonna be concerned by any code that start to manipulate or emit this. It'll have to be very contained.
The code Nicolas has uses platform-specific attributes, which are anyway strings that are not registered anyway. I don't see how to "fix" that without imposing our choices further down the pipeline. If someone suddenly starts to use this for an actual function attribute listed in LangRef, we will be able to model that attribute specifically, without having to prematurely overdesign all of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77072