Page MenuHomePhabricator

[mlir] Add fastmath flags support to some LLVM dialect ops
ClosedPublic

Authored by Hardcode84 on Dec 2 2020, 7:24 AM.

Details

Summary

Add fastmath enum, attributes to some llvm dialect ops, FastmathFlagsInterface op interface, and translateModuleToLLVMIR support.

Diff Detail

Event Timeline

Hardcode84 created this revision.Dec 2 2020, 7:24 AM
Hardcode84 requested review of this revision.Dec 2 2020, 7:24 AM
ftynse requested changes to this revision.Dec 2 2020, 9:13 AM

Thanks for the patch. I would recommend sending an RFC to https://llvm.discourse.group/ to discuss the proposed design. Personally, I would expect us to have something like a set of unit attributes with appropriate names as opposed to array-of-str-enum. It is not clear if we want a trait or an interface for this purpose (note that you cannot dyn_cast to a trait class, and unit attribute accessors are generated automatically).

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
53

Please document all top-level entities.

71

This is not an op interface (https://mlir.llvm.org/docs/Interfaces/), so the name is confusing.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2232

This looks like only one fastmath flag per op.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
671

Most of this file does the other way around: mlir names are used as is, LLVM names are explicitly prefixed.

This revision now requires changes to proceed.Dec 2 2020, 9:13 AM
mehdi_amini added inline comments.Dec 2 2020, 9:29 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
71

Yeah I'm the one who suggested an interface on Discord.
I think it fits nicely into an OpInterface.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2226

Nit: avoid trivial braces

2231

(please fix the warning)

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
709

(trivial braces)

Thanks for the patch. I would recommend sending an RFC to https://llvm.discourse.group/ to discuss the proposed design. Personally, I would expect us to have something like a set of unit attributes with appropriate names as opposed to array-of-str-enum. It is not clear if we want a trait or an interface for this purpose (note that you cannot dyn_cast to a trait class, and unit attribute accessors are generated automatically).

You mean separate attribute for each flag? In this case we will need a proper long name for each attribute (smth like fastmath-nnan) and it will look really cumbersome in text IR if you have multiple such attributes on some op.

But there is another issue with array-of-str approach actually, fastmath=["nnan", "ninf"] and fastmath=["ninf", "nnan"] are semantically equivalent but differs from MLIR IR POV, I don't know if this is important. I wish there were some sort of StringSet attribute type.

You can also model FMF as a custom Dialect attribute with its own printer/parser. That way the storage can be a bitfield in memory and you print/parse the string on the fly.

ftynse added a comment.Dec 3 2020, 8:12 AM

Thanks for the patch. I would recommend sending an RFC to https://llvm.discourse.group/ to discuss the proposed design. Personally, I would expect us to have something like a set of unit attributes with appropriate names as opposed to array-of-str-enum. It is not clear if we want a trait or an interface for this purpose (note that you cannot dyn_cast to a trait class, and unit attribute accessors are generated automatically).

You mean separate attribute for each flag? In this case we will need a proper long name for each attribute (smth like fastmath-nnan) and it will look really cumbersome in text IR if you have multiple such attributes on some op.

But there is another issue with array-of-str approach actually, fastmath=["nnan", "ninf"] and fastmath=["ninf", "nnan"] are semantically equivalent but differs from MLIR IR POV, I don't know if this is important. I wish there were some sort of StringSet attribute type.

Why would we need longer names than nnan and ninf? Dependent attributes are interpreted in the context of the operation they are attached to so I don't see any issues with that. llvm.fadd %0, %1 { nnan, ninf } : !llvm.floatis shorter than any other variant I can think of except for opaque integer/bitfield; it also happens to be one of the most storage- and lookup-efficient and the fact that attributes are treated as a dictionary solves your ordering problem.

You can also model FMF as a custom Dialect attribute with its own printer/parser. That way the storage can be a bitfield in memory and you print/parse the string on the fly.

This is also an option, not sure if the compression is worth the fuss though.

Hardcode84 edited the summary of this revision. (Show Details)

Updated. Went with custom attribute approach as there aren't any good decision how to handle build methods with bunch of unit attrs.

Some more comments:

  • Have to change bunch of unrelated tests to handle possible new ops attributes, I would like to avoid that but have no ideas how to do this
  • Used IntegerAttributeStorage for my bitfield attribute but had to do some ugly relative includes for AttributeDetail.h as it not part of public headers, any ideas how to this properly
  • It would be nice to have a way to get list of all enum values (tblgen generated)
  • DefaultValuedAttr require constBuilderCall to be present but not enforces that and tblgen just silently generate invalid C++ code in that case

The approach LGTM, please address the rest of the comments. Larger things:

  • let's just define a custom storage class, it's not that complex;
  • let's make the attribute optional, so the absence thereof indicates there are no flags;
  • we prefer longer, readable names to abbreviations, so fastmath or fastMathFlags whenever possible; fastmath = #llvm.fmf<nnan, ninf> in the attribute list is fine though, no need to repeat.
mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
55

You likely want a custom storage type here. IntegerAttributeStorage has a bunch of unnecessary complexity for this case -- storing long integers and associating a type. Just wrapping uint8_t in a storage class is largely sufficient here.

61

The convention is to prefix llvm:: types except for ADT that are re-exported into the mlir namespace.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
33

A better description is most welcome

109–110

Nit: could we have a more descriptive name? At least something like commonArgs, because opArgs is applicable to absolutely any op...

123

ODS produces functions that with the same name as $-variable, so I would prefer something more readable than fmf.

253

Maybe we can add the trait directly on LLVM_FloatArithmeticOp instead of repeating it here?

mlir/include/mlir/Dialect/LLVMIR/LLVMOpsInterfaces.td
2

Copy-pasta in the file name.

21

It is not an example.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1839–1843

It feels like adding an OpBuilderDAG with default value {] for the fastmath flags into the op definition would remove all of such boilerplate changes.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
14

The storage class you want to use is designed for built-in integer attributes, and may evolve with them. We don't really want to depend on it.

2248

Nit: I don't think the attr part brings anything to the syntax, we know its an attribute. By dropping it, we can also get a clearer and more pronounceable name - fastmast<>.

2258–2259

Please report an error here. Or, better, dispatch based on kind and report errors before even entering this function.

2265

Nit: if (failed(parser....)) reads better

2272–2273

Please report an error here.

mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
151–153 ↗(On Diff #311953)

I would have made the attribute optional (IIRC, it is possible to combine optional and default-valued in ODS), and these changes wouldn't have been necessary.

mlir/test/Target/llvmir.mlir
1362–1366

Could we please have more coverage? At least a couple of different ops, and all flags.

ftynse requested changes to this revision.Dec 16 2020, 1:26 AM
This revision now requires changes to proceed.Dec 16 2020, 1:26 AM

Why does this require a new attribute type? What is missing from the ODS enum generator?

let's make the attribute optional, so the absence thereof indicates there are no flags

It will work, but it will change accessors from FastmathFlags fmf() to Optional<FastmathFlags> fmf() making it less convenient and duplicating FastmathFlags(0) state. DefaultValuedAttr worked almost perfectly (e.g. it can handle missing attribute, returning default value), the only missing part is generated build methods, where desired behavior will be to store attribute only if it isn't equal to default value. Maybe extend DefaultValuedAttr with some additional parameter for this behavior?

Why does this require a new attribute type? What is missing from the ODS enum generator?

Storing bitflags attributes as list of readable strings in IR

let's make the attribute optional, so the absence thereof indicates there are no flags

It will work, but it will change accessors from FastmathFlags fmf() to Optional<FastmathFlags> fmf() making it less convenient and duplicating FastmathFlags(0) state. DefaultValuedAttr worked almost perfectly (e.g. it can handle missing attribute, returning default value), the only missing part is generated build methods, where desired behavior will be to store attribute only if it isn't equal to default value. Maybe extend DefaultValuedAttr with some additional parameter for this behavior?

Yes, it works, but in exchange you now need to always print the attribute on all ops, as you noticed yourself. Nobody obliges you to use the autogenerated accessor, you can easily add an additional function in the interface that removes the value contained in Optional, or the default value otherwise. (Arguably, we can also teach ODS backend to emit those directly for all optional+defaultvalued attributes, but that will cause some API churn).

I mean the only change I need is to have tblgen to generate

if (fmf != <default value>)
  odsState.addAttribute("fmf", ::mlir::LLVM::FMFAttr::get(fmf, odsBuilder.getContext()));

instead of unconditional

odsState.addAttribute("fmf", ::mlir::LLVM::FMFAttr::get(fmf, odsBuilder.getContext()));

in build methods for DefaultValuedAttr.
In that case IR with default flags should look like exactly as before without OptionalAttr.
Does it make sense?
Is it worth an RFC?

I mean the only change I need is to have tblgen to generate

if (fmf != <default value>)
  odsState.addAttribute("fmf", ::mlir::LLVM::FMFAttr::get(fmf, odsBuilder.getContext()));

instead of unconditional

odsState.addAttribute("fmf", ::mlir::LLVM::FMFAttr::get(fmf, odsBuilder.getContext()));

in build methods for DefaultValuedAttr.
In that case IR with default flags should look like exactly as before without OptionalAttr.
Does it make sense?
Is it worth an RFC?

We may have default-valued attributes that are not optional, i.e. the op semantics is to always have an attribute with a specific name. The optionality says the attribute may be totally absent. Default-valued optional attribute can be interpreted as "in case of absence, use this value". Actually, the default-valued thing is only an ODS sugar, the actual op does not even need to know whether how the attribute was defined in the builder. That's why I mentioned specifically attributes that are both optional and default-valued. Not sure if this needs an RFC, but it needs a different review thread because it might change how other ops print/parse.

Hardcode84 added inline comments.Wed, Dec 30, 4:17 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
53

Done, although I am very good at writing docs :)

61

This is our enum from LLVMOpsEnums, not LLVM

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
1839–1843

There aren't much boilerplate related to this specific case, maybe let it be that way for now?

Updated: make a custom printers which won't print fastmath flags if they are equal to the default value, to avoid changes in unrelated tests

ftynse added inline comments.Tue, Jan 5, 3:21 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
53

Please add trailing dots :)

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
83–84

We can call custom functions from assemblyFormat (https://mlir.llvm.org/docs/OpDefinitions/#custom-directives), wouldn't that be sufficient?

Hardcode84 added inline comments.Tue, Jan 5, 1:01 PM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
83–84

I didn't know that, fixed

ftynse accepted this revision.Wed, Jan 6, 1:52 AM

Thanks for iterating on this!

This revision is now accepted and ready to land.Wed, Jan 6, 1:52 AM

Can someone merge this, please?

ftynse added a comment.Thu, Jan 7, 4:26 AM

Can someone merge this, please?

Yes, please give me the name and email address you would like to be associated with the commit.

Ivan Butygin ivan.butygin@intel.com

This revision was automatically updated to reflect the committed changes.
ftynse added a comment.Thu, Jan 7, 5:00 AM

FYI, if you send commits with arcanist, the committer credentials are preserved.