This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add missing properties on llvm.x86.sse.{st,ld}mxcsr
ClosedPublic

Authored by courbet on Jun 5 2019, 3:03 AM.

Details

Summary

llvm.x86.sse.stmxcsr only writes to memory.
llvm.x86.sse.ldmxcsr only reads from memory, amd might generate an FPE.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Jun 5 2019, 3:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 3:03 AM

There are a bunch of tests to update, but I'd like to get opinions on this before going further. I'm not sure why the properties were not already there. Did I miss something ?

Don’t we need stmxcsr to have side effects to keep it from reordering relative to ldmxcsr?

courbet added a comment.EditedJun 5 2019, 8:59 AM

Don’t we need stmxcsr to have side effects to keep it from reordering relative to ldmxcsr?

Reordering should be prevented by ldmxcsr having side effects, right ?
Essentially stmxcsr is nothing more than a fancy MOV32mr.

I don't think that's sufficient. A side effecting instruction doesn't prevent other non-side effecting instructions from moving across it. It only prevents other side effecting instructions from moving relative to it. If you load mxcsr from some address A, then store mxcsr to some address B. Without having side effects on the stmxcsr, nothing would prevent it from being able to move above the ldmxcsr.

courbet added a comment.EditedJun 5 2019, 9:09 AM

A side effecting instruction doesn't prevent other non-side effecting instructions from moving across it.

OK, that's where I was making incorrect assumptions. Thanks.

courbet updated this revision to Diff 203344.Jun 6 2019, 6:33 AM
  • Mark ldmxscr as writing to memory. Unfortunately the DAG relies on intrinsics writing to memory to determine side-effet-ness.
  • Mark stmxscr as having side effects, as discusse din the review.
  • Update tests.
courbet updated this revision to Diff 203345.Jun 6 2019, 6:36 AM

Fix formatting

Harbormaster completed remote builds in B32986: Diff 203345.
RKSimon added inline comments.Jun 15 2019, 6:24 AM
llvm/include/llvm/IR/IntrinsicsX86.td
290 ↗(On Diff #203345)

@craig.topper Are you OK with this?

This revision is now accepted and ready to land.Jun 18 2019, 10:55 PM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.