This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Improve diagnostics for vectors with incorrect element-size.
ClosedPublic

Authored by sdesmalen on May 10 2018, 3:57 AM.

Details

Summary

For regular SVE vector operands, this patch introduces a more
sensible diagnostic when the vector has a wrong suffix (e.g. z0.s vs z0.b).

For example:

add z0.s, z1.s, z2.b      -> invalid element width
             ^_____^
             mismatch

For the vector-with-shift/extend (e.g. z0.s, uxtw #2) this patch takes
a slightly different approach and instead returns a 'invalid operand'
if the element size is not as expected. This is because the diagnostics
are more specificied to suggest using the right shift/extend suffix. This
is a trade-off not to introduce more operand classes and still provide
useful diagnostics for LD1 and PRF instructions.

For example:

ld1w z1.s, p0/z, [x0, z0.s] -> invalid shift/extend specified, expected 'z[0..31].s, (uxtw|sxtw)'
ld1w z1.d, p0/z, [x0, z0.s] -> invalid operand
        ^________________^
             mismatch

For gather prefetches, both 'z0.s' and 'z0.d' would be allowed:

prfw #0, p0, [x0, z0.s]   -> invalid shift/extend specified, expected 'z[0..31].s, (uxtw|sxtw) #2'
prfw #0, p0, [x0, z0.d]   -> invalid shift/extend specified, expected 'z[0..31].d, (lsl|uxtw|sxtw) #2'

Without this change, the diagnostic would unnecessarily suggest a
different element size:

prfw #0, p0, [x0, z0.s]   -> invalid shift/extend specified, expected 'z[0..31].d, (lsl|uxtw|sxtw) #2'

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.May 10 2018, 3:57 AM
SjoerdMeijer added inline comments.May 10 2018, 4:05 AM
test/MC/AArch64/SVE/ld1b-diagnostics.s
118 ↗(On Diff #146107)

Is this a regression? Is the old diagnostic more accurate? If so, can we avoid this regression? There is quite a few of them below.

sdesmalen added inline comments.May 10 2018, 4:59 AM
test/MC/AArch64/SVE/ld1b-diagnostics.s
118 ↗(On Diff #146107)

It is, but a conscious one. I tried to explain it a bit in the description of the patch, but I can understand this may not have been very clear :)
This regression is a trade-off to get better diagnostics for the prefetches. We could avoid the regression if we create new operand classes for each shift/extend where it makes the distinction between "both z0.s and z0.d are allowed in this position", and "only z0.d is allowed". That way, the assembler can make the choice what diagnostic to emit. However, I think that would (over)complicate the operand classes.
Please have a second look at the commit message to see some examples of this trade-off and let me know what you think is the best way forward.

SjoerdMeijer accepted this revision.May 10 2018, 6:14 AM
SjoerdMeijer added inline comments.
test/MC/AArch64/SVE/ld1b-diagnostics.s
118 ↗(On Diff #146107)

Ah, sorry, I guess I didn't read the description carefully enough!

I didn't count the number of regressions, but this change looks neutral: there are as many regressions in the diagnostics as there are improvements. Perhaps there are slightly more improvements.

The royal approach of course would be to have the best of both worlds (i.e. no regressions), but you write that would require more operands classes. I don't know if that would make things messy or not, but given that the change is neutral, I can live with this change. Perhaps you can wait a day before committing just in case someone can't.

This revision is now accepted and ready to land.May 10 2018, 6:14 AM
This revision was automatically updated to reflect the committed changes.