This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Fastmath flags import from LLVM IR.
ClosedPublic

Authored by gysit on Dec 8 2022, 4:08 AM.

Details

Summary

This revision adds support to import fastmath flags from LLVMIR. It
implement the import using a listener attached to the builder. The
listener gets notified if an operation is created and then checks if
there are fastmath flags to import from LLVM IR to the MLIR. The
listener based approach allows us to perform the import without changing
the mlirBuilders used to create the imported operations.

An alternative solution, could be to update the builders so that they
return the created operation using FailureOr<Operation*> instead of
LogicalResult. However, this solution implies an LLVM IR instruction
always maps to exatly one MLIR operation. While mostly true, there are
already exceptions to this such as the PHI instruciton. Additionally, an
mlirBuilder based solution also further complicates the builder
implementations, which led to the listener based solution.

Depends on D139405

Diff Detail

Event Timeline

gysit created this revision.Dec 8 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Dec 8 2022, 4:08 AM
ftynse added a comment.Dec 9 2022, 3:05 AM

I'm not entirely convinced by the listener-based approach. It feels hacky because (1) action-at-a-distance on the IR that is not visible at the construction point and (2) one doesn't expect a thing called a listener to patch up what is created by the builder itself rather than notifying something external. I'm willing to accept that this is a better alternative to updating all relevant builder snippets, but this needs significantly better documentation. Literally putting // fastmast flags are handled by the listener in processInstruction in all relevant operations, and having this mentioned in some overview documentation.

gysit added a comment.Dec 9 2022, 3:30 AM

I'm not entirely convinced by the listener-based approach.

I understand the concern that this is somewhat in-transparent.

I think there are two problems here: 1) one is using the listener to obtain the created operation rather than returning the created operation from the mlirBuilder and 2) is modifying the imported operation outside of the builder at all. I think 1) could be addressed by returning FailureOr<Operation*> from the builder. I am happy to try this out! I think the problem there is that it somewhat conflicts with the current form of the result mapping in the builders (I believe this can be addressed). Concern 2) is a bit harder to address except the fastmath flag import is done in all the builders (I think that way we cannot really make use of the fastmath interface to automatically import the flags, which IMO is quite an elegant solution?). Additionally, I was thinking of using such a mechanism to make the import extensible. I.e., it would be very nice if a downstream project could extend the import by registering a callback that takes an llvm instruction and the according mlir operation. The callback could then, for example, import custom metadata / attributes without any need for updating the upstream import. Of course such a mechanism needs good documentation and it may not be desired at all...

Would returning the Operation and attaching the fastmath attributes after the builder call (probably still in processInstruction) be an improvement from you point of view?

gysit updated this revision to Diff 482006.Dec 11 2022, 11:49 PM

Address reviewer comment and implement a new variant of the revision that
is based on modifying the builders. It turns out the builder modifications
are not too complex and they make fastmath import very explicit. If we
want to reduce the builder complexity we could also do this without
a listener since all fastmath operations return a result which would
be accessible from outside the builder. The listener based approach
does definitely seems like a hacky solution for this problem.

ftynse accepted this revision.Dec 15 2022, 8:29 AM

Nice! Thanks for iterating on the design!

This revision is now accepted and ready to land.Dec 15 2022, 8:29 AM
This revision was automatically updated to reflect the committed changes.