Page MenuHomePhabricator

[ARM] Issue an eror when non-general-purpose registers used in address operands (alternative)
ClosedPublic

Authored by chill on Nov 10 2017, 10:58 AM.

Details

Summary

Currently the assembler will happily assemble, e.g. ldr r0, [s0, #12] and similar.
This patch add checks that only general-purpose registers are used in address operands, shifted registers, and shift amounts.
The check are done in the various AsmOperandClass predicates, which seems simple and as the right place to do it, but
the error messages aren't that good as in the alternative patch here: https://reviews.llvm.org/D39909

Event Timeline

chill created this revision.Nov 10 2017, 10:58 AM
chill added a comment.Nov 14 2017, 9:13 AM

Correct me if I'm wrong ...

For me it seems that the approach of returning just true/false from the asm operand class predicates, while
good for weeding out invalid instructions, isn't that great for diagnostics. The operand may be invalid for
a variety of reasons, but we have only one diagnostics string, which can at best list all the possible constraints
and let the user figure out by themselves what's wrong.

For example, assembling:

vld1.16 {d0}, [s0:64]

with llvm-mc -triple=armv7a-eabi gives error: alignment must be 64 or omitted, and if we need to improve
this message, we'd have to list the constrain that the register must be GPR. And do that for all/lot-of the other
AsmOperandClassses, which would create a huge number of repetetetetitions of the constraint "X must be a GPR".
If we instead diagnose usage of non-GPR registers in the parser, and leave checking more specific constraints
(like "X must not be PC" or "X must be SP") to asm operand class predicates, some diagnostics will be more
precise and the rest will require less effort by the user, because that'd list a comparatively small number of
constraints to manually check.

Having said that, I still tried this approach to a few asm operand classes and the results look promising.

Anyway, I suggest separating the correctness check for general-purpose registers from a set of follow-up of patches, which
just improve the error message "invalid operand for instruction".

chill updated this revision to Diff 123317.Nov 17 2017, 4:13 AM

Updated the asm operand diagnostic messages.

olista01 edited edge metadata.Nov 21 2017, 10:47 AM

It looks like there are a lot of inaccuracies here still with regards to accepting r13/sp and r15/pc. Rejecting non-GPRs is obviously an improvement, but I don't think we should be adding diagnostics which give wrong information to the user.

Maybe the best course of action here would be to commit just the part restricting registers to GPR now, leaving the diagnostics as "invalid operand". We can then fix the operand classes precisely a few at a time, adding predicate functions and diagnostics that match.

lib/Target/ARM/ARMInstrInfo.td
608 ↗(On Diff #123317)

I've been trying to standardise the grammar of these messages on "operand must be a <type> ..." rather than "<type> operand must be ...", see D36689.

609 ↗(On Diff #123317)

Neither register can be r15, at least for ADD on v7A and v8A.

939 ↗(On Diff #123317)

The index register can't be r15 (at least for ARM LDR on v7A).

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
1157 ↗(On Diff #124377)

There is no code here checking for anything more specific than GPR (other than the TBB/TBH operands), but there are lots of instructions that cannot use sp or pc.

chill added inline comments.Nov 22 2017, 4:01 AM
lib/Target/ARM/ARMInstrInfo.td
608 ↗(On Diff #123317)

The reason for this phrasing is that it makes the diagnostic more clear, for example, when assembling

add r3, r0, s1, lsl r6

the error message is like

addr.s:17:17: note: shifted register operand must use two registers in the range [r0, r15]
add r3, r0, s1, lsl r6
                ^

with the caret pointing to the shift amount, whereas it's the s1 register that needs to be fixed.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
1157 ↗(On Diff #124377)

Right. So, I have to draw the line somewhere and currently it's at not allowing non-GPRs in address and shifted register operands, and updating the diagnostics to match the code (in that sense, I probably shouldn't have made the changes for TBB/TBH).

Perhaps we would like to reject syntaxes, which generate an encoding with unpredictable behavior, or perhaps it's not worth the trouble. To me it seems like a subject for further discussion and a follow-up patch(es).

chill added a comment.Nov 22 2017, 4:03 AM

I'll be happy going forward without the diagnostics changes, though.

rengolin requested changes to this revision.Nov 26 2017, 3:51 AM

I think this change goes against the recent trend of printing multiple diagnostics, instead of a huge dump that will certainly be inaccurate at least some times.

I agree with @olista01 that terse messages are better than wrong messages. If there is inaccuracies, let's fix them, but this patch is way beyond that.

This revision now requires changes to proceed.Nov 26 2017, 3:51 AM
chill updated this revision to Diff 124342.Nov 27 2017, 3:21 AM
chill edited edge metadata.

Reverted to the first variant of the patch.

This patch doesn't have full context (git diff -U999999), please use that in future as it makes reviewing easier.

test/MC/ARM/arm-reg-addr-errors.s
2 ↗(On Diff #124377)

The previous version of this patch also tested Thumb, and FP/SIMD loads and stores. The code change looks like it should still affect those (except for the diagnostic strings), so could those tests also be kept?

chill added a comment.Nov 27 2017, 7:29 AM

The preceding version contained more tests as they were covering each affected AsmOperandClass. Without the error messages, the tests just cover the changed functions, i.e. adding a test for vld4.32 {d0[], d1[], d2[], d3[]}, [s7:64]
won't test more than the already tested isMem().
I'll re-upload the diff with more context.

chill updated this revision to Diff 124377.Nov 27 2017, 7:30 AM

Updated the diff to include full context.

olista01 accepted this revision.Nov 28 2017, 1:51 AM

I approved this patch back in November.

I approved this patch back in November.

Just as an FYI, Phab won't send an email to let folks know about this unless you actually write something in the comment box.

When marking patches as approved, try to always write "LGTM" or something in the message.

chill added a comment.Jan 5 2018, 3:16 AM

I approved this patch back in November.

Just as an FYI, Phab won't send an email to let folks know about this unless you actually write something in the comment box.

When marking patches as approved, try to always write "LGTM" or something in the message.

chandlerc, thanks. I got all the emails, and was well aware about the status of the review at all time, just wasn't sure what is the proper way to proceed when having conflicting reviews.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 5 2018, 5:29 AM
This revision was automatically updated to reflect the committed changes.