Page MenuHomePhabricator

MachineInst support mapping SDNode fast math flags for support in Back End code generation
ClosedPublic

Authored by mcberg2017 on Apr 18 2018, 11:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mcberg2017 created this revision.Apr 18 2018, 11:31 AM
mcberg2017 edited the summary of this revision. (Show Details)Apr 18 2018, 11:33 AM
mcberg2017 added reviewers: spatel, arsenm.
mcberg2017 added inline comments.Apr 18 2018, 11:35 AM
include/llvm/CodeGen/MachineInstr.h
110 ↗(On Diff #142973)

Does this change the overall size of the MachineInstr struct, or is there already enough padding to use?

mcberg2017 added inline comments.Apr 18 2018, 12:10 PM
include/llvm/CodeGen/MachineInstr.h
110 ↗(On Diff #142973)

The padding in the MIFlag enum is such that the new flags do not change the size of the class, however, the Flags field changes it by 1 byte in size.

mcberg2017 marked 2 inline comments as done.Apr 18 2018, 12:10 PM

This is missing the support and tests in the MIR printer for these

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
828–834 ↗(On Diff #142973)

This section belongs with the DAG patch

Meantime, I will look into the tests and MIR printer changes.

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
828–834 ↗(On Diff #142973)

That would create a circular dependence.

arsenm added inline comments.Apr 23 2018, 11:31 AM
lib/CodeGen/SelectionDAG/InstrEmitter.cpp
828–834 ↗(On Diff #142973)

How? You should be able to make all the MI changes first, independent of the DAG

As per request, I have moved lib/CodeGen/MachineInstr.cpp to D45710, also added printer code for MI. Tests were requested as well, however they will appear with all the other functional aspects of these changes in D45710, as the full set of changes is needed for test modifications.

What other changes are necessary? This should be sufficient to test the MIR parser

It should be noted with the changes above, that this change list is now required to check in first before D45710 and that there is a hard dependency on this code.

mcberg2017 edited the summary of this revision. (Show Details)Apr 24 2018, 3:08 PM

The code in lib/CodeGen/SelectionDAG/InstrEmitter.cpp sets the flags, which will shortly be D45710, without that, nothing changes in the mir tests, ergo the check component testing for the fast math flags will cause modified tests which run through the MIR printer to fail until those changes are checked in.

The code in lib/CodeGen/SelectionDAG/InstrEmitter.cpp sets the flags, which will shortly be D45710, without that, nothing changes in the mir tests, ergo the check component testing for the fast math flags will cause modified tests which run through the MIR printer to fail until those changes are checked in.

If the instructions are codegened from SelectionDAG. This seems to be missing the part to parse these names and set the flags in the MIR parser, which is what should be setting the flags here.

Ok, I will add the parser/lexer pieces to this review, once I have them authored/tested.

Added mir synchronization support for MI fast math flags with a test.

Are there any outstanding issues regarding these changes? Does it look ok for approval?

arsenm added inline comments.Apr 30 2018, 11:58 AM
test/CodeGen/X86/sqrt-fastmath.mir
1 ↗(On Diff #143855)

This test belongs in test/CodeGen/MIR/X86. The sqrt also isn't needed in the name, just fast-math-flags would be better

8–12 ↗(On Diff #143855)

Don't need the IR section

16–17 ↗(On Diff #143855)

You can strip out most of this stuff

74–75 ↗(On Diff #143855)

This doesn't need to be a meaningful function. You can just have one sample instruction for each individual flag, and maybe a few with combination flags for good measure

mcberg2017 marked 4 inline comments as done.

MIR test adjusted, please have a look.

arsenm accepted this revision.May 2 2018, 4:54 AM

LGTM

This revision is now accepted and ready to land.May 2 2018, 4:54 AM
This revision was automatically updated to reflect the committed changes.