This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Asm: Support for FDUP_ZI (copy fp immediate) instruction.
ClosedPublic

Authored by sdesmalen on May 29 2018, 8:58 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.May 29 2018, 8:58 AM
fhahn added a comment.Jun 1 2018, 1:43 AM

Looks good, but do the tests have to be that big? That makes it really hard to spot if the edge cases & aliases are handled properly. Can you reduce the size a bit?

Looks good, but do the tests have to be that big? That makes it really hard to spot if the edge cases & aliases are handled properly. Can you reduce the size a bit?

Since the immediate encoding only allows for a limited set of 256 different floating-point immediates, I thought it would make sense to test each of them separately to make sure they are all correctly parsed, assembled and printed, since basically these are all the edge-cases. The negative tests are much smaller and test a few floating point values that cannot be encoded. I can easily reduce the tests, but I think these tests are valuable. What do you think?

fhahn added a comment.Jun 1 2018, 2:13 AM

Hm we have much smaller test cases for other instructions with similarly limited immediates. I think it is just unlikely that someone will very carefully review 3000 lines of test changes. Especially for FDUP which is just an alias for FMOV, testing each immediate seems slightly excessive to me.

I think the difference with other immediates that have a limited set, is that these immediates have a non-trivial parsing/decoding/printing. Rather than just allowing a given range of integer values the string is first parsed as FP value and then encoded as integer value between 0-255. And the opposite for the decode. I think it makes sense to test these cases individually, although I can see how doing this for the FMOV alias as well could be a bit excessive, so I'll reduce those.

The assembler/disassembler tests are mostly there to test against regression and I don't really expect anyone to check/update the encoding/decoding. This as opposed to the negative-tests, which may be updated to provide more sensible diagnostics.

fhahn added a comment.Jun 1 2018, 3:30 AM

I think the difference with other immediates that have a limited set, is that these immediates have a non-trivial parsing/decoding/printing. Rather than just allowing a given range of integer values the string is first parsed as FP value and then encoded as integer value between 0-255. And the opposite for the decode. I think it makes sense to test these cases individually, although I can see how doing this for the FMOV alias as well could be a bit excessive, so I'll reduce those.

Thanks, that makes sense.

sdesmalen updated this revision to Diff 149435.Jun 1 2018, 5:42 AM

Removed some redundant test-cases for fmov alias that are already tested by test/MC/AArch64/SVE/fdup.s

fhahn accepted this revision.Jun 1 2018, 5:43 AM

LGTM thanks!

This revision is now accepted and ready to land.Jun 1 2018, 5:43 AM
This revision was automatically updated to reflect the committed changes.