Page MenuHomePhabricator

[X86] Model MXCSR for all AVX512 instructions
ClosedPublic

Authored by pengfei on Sun, Dec 1, 6:10 PM.

Details

Summary

Model MXCSR for all AVX512 instructions

Event Timeline

pengfei created this revision.Sun, Dec 1, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Dec 1, 6:10 PM

My biggest concern here is that the SAE and implicit rounding instructions are all marked as mayRaiseExceptions which is incorrect.

llvm/lib/Target/X86/X86InstrInfo.cpp
1764

Use getNumExplicitOperands() - 1 instead.

pengfei updated this revision to Diff 231651.Mon, Dec 2, 2:12 AM
pengfei marked an inline comment as done.

Address review comments.

Removed MXCSR and mayRaiseExceptions from SAE opcodes.

llvm/lib/Target/X86/X86InstrInfo.cpp
1764

Thanks!

I think SAE instructions and embedded rounding instructions still read the DAZ and FTZ bits from MXCSR

This comment was removed by pengfei.

I think SAE instructions and embedded rounding instructions still read the DAZ and FTZ bits from MXCSR

I think so, but currently we don't model DAZ and FTZ, right?

The comment in X86RegisterInfo.td says that, but where was that discussed?

I think SAE instructions and embedded rounding instructions still read the DAZ and FTZ bits from MXCSR

I think so, but currently we don't model DAZ and FTZ, right?

The comment in X86RegisterInfo.td says that, but where was that discussed?

You are correct, we do need to model DAZ and FTZ. I think I commented "not modeled" just for simplification. I will commit a patch to model them.

craig.topper added inline comments.Mon, Dec 2, 8:51 PM
llvm/lib/Target/X86/X86InstrAVX512.td
7121

Don't we need to suppress MXCSR and exceptions here

llvm/lib/Target/X86/X86InstrInfo.cpp
1764

Is this line longer than 80 columns?

pengfei updated this revision to Diff 231819.Mon, Dec 2, 9:31 PM
pengfei marked an inline comment as done.

Address review comments.

pengfei marked an inline comment as done.Mon, Dec 2, 9:31 PM
pengfei added inline comments.
llvm/lib/Target/X86/X86InstrAVX512.td
7121

Yes, I missed it. Thanks!

llvm/lib/Target/X86/X86InstrInfo.cpp
1764

Fixed, thanks!

This revision is now accepted and ready to land.Tue, Dec 3, 2:48 PM
This revision was automatically updated to reflect the committed changes.