This is an archive of the discontinued LLVM Phabricator instance.

[X86] Model MXCSR for all SSE instructions
ClosedPublic

Authored by craig.topper on Sep 26 2019, 11:37 PM.

Details

Summary

This patch adds MXCSR as a reserved physical register and models its use
by X86 SSE instructions. It also adds flag "mayRaiseFPException" for the
instructions that possibly can raise FP exception according to the
architecture definition.

Following what SystemZ and other targets does, only the current rounding
modes and the IEEE exception masks are modeled. *Changes* of the MXCSR
due to exceptions are not modeled.

Event Timeline

pengfei created this revision.Sep 26 2019, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 11:37 PM

Does this really not affect any tests? I would have thought some test would print MIR output that would show the implicit use that wasn't there before.

llvm/lib/Target/X86/X86InstrFormats.td
230

Is EXP an abbreviation for Exception? If so, I think EXC is probably a better abbreviation

Instead of attributing all these instructions with MXCSR first wouldn't we be better off starting just attributing (V)LDMXCSR/(V)STMXCSR in this patch and then future patches sets up groups of instructions with suitable tests?

llvm/lib/Target/X86/X86InstrFormats.td
230

mayRaiseMXCSRException might be better as well - to avoid x87/fpu ambiguity

pengfei marked an inline comment as done.Sep 27 2019, 5:43 AM

Instead of attributing all these instructions with MXCSR first wouldn't we be better off starting just attributing (V)LDMXCSR/(V)STMXCSR in this patch and then future patches sets up groups of instructions with suitable tests?

The difficult is we may have X87/SSE/AVX/AVX512 instructions under the same node, so I'm planning to mode the attribute of instructions by X87/SSE/AVX/AVX512 first, then set up instructions by node.
And we don't need to model MXCSR for (V)LDMXCSR/(V)STMXCSR, because they are using hasSideEffects and cannot be replaced due to we only modeling rounding and exception bits in MXCSR.

llvm/lib/Target/X86/X86InstrFormats.td
230

mayRaiseFPException is a target independent flag and is widely used to check if an instruction may raise exception during optimization. I think there's no need to distinguish it from x87 or SIMD during the optimization, and we can distinguish them from the cpcode of MI if necessary.

pengfei marked an inline comment as done.Sep 27 2019, 5:49 AM

Does this really not affect any tests? I would have thought some test would print MIR output that would show the implicit use that wasn't there before.

You are correct. Besides some MIR tests, there're also some machine combiner failure. I'm trying to fix it. Thanks for reminding.

llvm/lib/Target/X86/X86InstrFormats.td
230

Yes, I'll change the name.

pengfei updated this revision to Diff 222314.Sep 29 2019, 12:37 AM

Address review comments, add affected tests.

craig.topper added inline comments.Sep 29 2019, 1:42 AM
llvm/test/CodeGen/X86/evex-to-vex-compress.mir
2317 ↗(On Diff #222314)

Why are only COMI instructions updated?

pengfei marked an inline comment as done.Sep 29 2019, 6:45 AM
pengfei added inline comments.
llvm/test/CodeGen/X86/evex-to-vex-compress.mir
2317 ↗(On Diff #222314)

We don't have any special test case for checking the operands of MIs, and I checked other target don't either. This file may be the one that checks most AVX/AVX512 MIs, but we only modeled SSE instructions in this patch.
VCOMI shouldn't be updated either. The reason is VCOMISS/VCOMISD are defined is X86InstrSSE.td and using sse12_ord_cmp which I modeled MXCSR in.
In fact, I just made sure all SSE instructions are correctly modeled, not all modeled instructions are SSE. I think it doesn't matter, because it is NFC and we will model AVX instructions next.

And we don't need to model MXCSR for (V)LDMXCSR/(V)STMXCSR, because they are using hasSideEffects and cannot be replaced due to we only modeling rounding and exception bits in MXCSR.

I'm not talking about removing the hasSideEffects tag (at least not yet), I'm just suggesting that it'd make sense to indicate that MXCSR is written/read by (V)LDMXCSR/(V)STMXCSR

craig.topper commandeered this revision.Oct 30 2019, 2:19 PM
craig.topper edited reviewers, added: pengfei; removed: craig.topper.

Commandeering so I can rebase part of this patch to remove the X86InstrInfo.cpp changes.

Remove X86InstrInfo.cpp changes

This revision is now accepted and ready to land.Oct 30 2019, 2:40 PM
This revision was automatically updated to reflect the committed changes.