This is an archive of the discontinued LLVM Phabricator instance.

Mapping SDNode flags to MachineInstr flags
ClosedPublic

Authored by mcberg2017 on May 4 2018, 12:27 PM.

Details

Summary

Providing the glue to map SDNode fast math sub flags to MachineInstr fast math sub flags.

Diff Detail

Event Timeline

mcberg2017 created this revision.May 4 2018, 12:27 PM
mcberg2017 added reviewers: spatel, arsenm.
spatel accepted this revision.May 4 2018, 2:31 PM

The transfer itself is straightforward, so LGTM.

But if you can explain here for the record how we get from the IR FMF and function attributes to the DAG flags, that would be good. (And yes, I know you have some of the patches in progress to improve this.)

This revision is now accepted and ready to land.May 4 2018, 2:31 PM
mcberg2017 added a comment.EditedMay 4 2018, 2:37 PM

As part to the global explanation behind the design of this and other components in this stream of reviews, fast math flags are mined directly off of IR and propagated (or will be more so incrementally over time), so that we can rely on similar functionality without the use of the module level unsafe flag. The goal is to achieve parity in code generation and to propagate context sensitive guards over optimization and code generation so that relative functional IR attributes control these features. As in the summary this patch provides the mapping off of SDNode into MachineInstr so that we carry full context at the IR level from high level compilation into back ends.

spatel added a comment.May 4 2018, 2:49 PM

As part to the global explanation behind the design of this and other components in this stream of reviews, fast math flags are mined directly off of IR and propagated (or will be more so incrementally over time), so that we can rely on similar functionality without the use of the module level unsafe flag. The goal is to achieve parity in code generation and to propagate context sensitive guards over optimization and code generation so that relative functional IR attributes control these features. As in the summary this patch provides the mapping off of SDNode into MachineInstr so that we carry full context at the IR level from high level compilation into back ends.

I was actually more concerned with the non-obvious outcome in the affected regression tests because this particular case (sqrt estimate) just came up in:
https://bugs.llvm.org/show_bug.cgi?id=37344

In the 1st test (plain sqrt estimate), there's only a function attribute. The estimate transform is predicated on that function attribute and then applies 'reassoc' to new nodes as much as it could at the time it was implemented. That means it set that flag for binops (fadd/fmul), but not fma or rsqrt!

In the 2nd test (reciprocal sqrt estimate), the fdiv is marked 'fast', so the transform is able to propagate that to all of the created binop nodes (again, the 'fma' get nothing currently, but they should).

In summary, it's full of bugs.

This revision was automatically updated to reflect the committed changes.