Page MenuHomePhabricator

[AArch64] fjcvtzs,rmif,cfinv,setf* all clobber nzcv
ClosedPublic

Authored by jroelofs on Jul 14 2020, 2:52 PM.

Details

Summary

Adding these defs will prevent these instructions from being scheduled within an nzcv live range.

Diff Detail

Event Timeline

jroelofs created this revision.Jul 14 2020, 2:52 PM

@SjoerdMeijer mind double-checking me on this?

Agreed. From the ArmARM:

Floating-point FJCVTZS instruction sets the PSTATE.Z flag if the result of the conversion, when converted back to a double-precision floating-point number, gives precisely the same value as the original. Other PSTATE flags are cleared by this instruction

And there are similar descriptions for the other instructions.
With the instruction descriptions fixed, would it be possible to add a test case for this:

Adding these defs will prevent these instructions from being scheduled within an nzcv live range.

jroelofs updated this revision to Diff 280232.Jul 23 2020, 12:56 PM

Add a test for the changes for fjcvtzs.

@SjoerdMeijer can you suggest a good way to write a test for the others? llvm-mc doesn't seem to have a way to print implicit defs/uses.

SjoerdMeijer added inline comments.Jul 24 2020, 4:10 AM
llvm/test/CodeGen/AArch64/fjcvtzs.ll
2

Testing this is probably a bit convenient, but I was thinking about a MIR test indeed, that should display all the implicit defs/uses.
This almost looks like a mir test, but doing it here in this way with llc and -stop-after is probably unconventional, see

https://llvm.org/docs/MIRLangRef.html#testing-individual-code-generation-passes

for creating a .mir test.

SjoerdMeijer added inline comments.Jul 24 2020, 4:15 AM
llvm/test/CodeGen/AArch64/fjcvtzs.ll
2

"Testing this is probably a bit convenient"
->
inconvenient

jroelofs updated this revision to Diff 280467.Jul 24 2020, 8:18 AM

Add mir tests for each of the affected instructions.

jroelofs updated this revision to Diff 280473.Jul 24 2020, 8:52 AM
SjoerdMeijer added inline comments.Jul 24 2020, 8:58 AM
llvm/test/CodeGen/AArch64/fjcvtzs.mir
1 ↗(On Diff #280467)

I am very sorry, looking at this, I am changing my mind on this. This test is not testing anything, there are no CHECK tags. And in fact, what you had before is better and what we want: an llc test fed with an .ll, using stop-after to produce mir and check for implicit defs/uses.

llvm/test/CodeGen/AArch64/flag-mainpulation.mir
1 ↗(On Diff #280467)

nit: typo in the file name: flag-mainpulation.mir

jroelofs marked an inline comment as done.Jul 24 2020, 9:25 AM
jroelofs added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1042

Disappointingly, these new tests don't fail if I remove these Defs/Uses, so they're not really effective at enforcing that these be here. The "unconventional" test I had before did that correctly, but I'm not sure how to write one for this group of instructions, since they're not matched by any PatFrag, and don't have corresponding intrinsics.

SjoerdMeijer added inline comments.Jul 24 2020, 9:29 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1042

Okay, thanks for trying. I think we've really tried, a change without tests here seems reasonable then.

jroelofs marked an inline comment as done.Jul 24 2020, 9:39 AM
jroelofs added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1042

Ooh, it just occurred to me that there is a way to test this: FileCheck for the error messages with mir that doesn't compile because the uses/defs are missing.

jroelofs marked an inline comment as done.Jul 24 2020, 9:41 AM
jroelofs added inline comments.
llvm/test/CodeGen/AArch64/fjcvtzs.mir
1 ↗(On Diff #280467)

No worries. It's good for me to learn new tools!

jroelofs updated this revision to Diff 280508.Jul 24 2020, 10:03 AM
jroelofs updated this revision to Diff 280509.

rm subsumed test

Kinda verbose, but it works. This would be so much easier if we had a way to dump mir from llvm-mc.

SjoerdMeijer accepted this revision.Jul 24 2020, 1:37 PM

Nice one! LGTM

Optional: since they are really small tests, you can try if it is convenient to have them all in one file, but separate is fine too I think.

This revision is now accepted and ready to land.Jul 24 2020, 1:37 PM

Optional: since they are really small tests, you can try if it is convenient to have them all in one file, but separate is fine too I think.

I wanted that, but couldn't find a way to make it work. The MIRParser quits after the first error :(

This revision was automatically updated to reflect the committed changes.