This is an archive of the discontinued LLVM Phabricator instance.

[TableGen][AsmMatcherEmitter] Only choose specific diagnostic for enabled instruction
ClosedPublic

Authored by sdesmalen on Nov 22 2017, 9:30 AM.

Details

Summary

When emitting a diagnostic for an invalid operand, a specific diagnostic
should only be reported when the instruction being matched is actually
enabled by the feature flags.

Patch [3/4] in a series to add parsing of predicates and properly parse SVE
ZIP1/ZIP2 instructions. This patch fixes bogus diagnostic messages for when
the SVE feature is not specified.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 22 2017, 9:30 AM
fhahn added a subscriber: fhahn.Nov 29 2017, 9:18 AM
olista01 accepted this revision.Dec 4 2017, 9:14 AM

This is very similar to what I'm doing with the multiple-near-miss diagnostics, trying to only emit messages which point out exactly what is wrong with an instruction, rather than simply the first thing the assembler found wrong. Hopefully this will be made obsolete once I get the near-miss mechanism completed and ported to non-ARM targets, but this looks like a reasonable point fix in the meantime. LGTM.

This revision is now accepted and ready to land.Dec 4 2017, 9:14 AM
rengolin edited edge metadata.Dec 6 2017, 1:58 AM

I'd wait someone from Mips to review the patch before committing, as the messages change a lot and I have no idea how valid they are.

@sdardis ?

sdardis edited edge metadata.Dec 6 2017, 3:22 AM

Thanks @renato for reminding of this patch. I'm a little concerned as some of the changes to the error messages are inconsistent. I'll take a longer look at this shortly but I've made some highlights below.

test/MC/Mips/micromips/invalid-wrong-error.s
9–13

These are improvements.

test/MC/Mips/micromips32r6/invalid-wrong-error.s
9–25

These all look like improvements.

test/MC/Mips/micromips64r6/invalid-wrong-error.s
18–51 ↗(On Diff #123961)

These all look like improvements.

test/MC/Mips/mips32r6/invalid-mips5-wrong-error.s
14

The error message here has improved. Unfortunately as we're rejecting the operand we aren't identifying which part of the operand is incorrect, but that's an issue for another day.

test/MC/Mips/mips64/invalid-mips64r2.s
32–41

This is strictly a regression in terms of error quality. The ins* family of instructions have a number of non-intuitive constraints on the ranges of their immediate operands. "invalid operand for instruction" doesn't indicate what the operand should even be, whereas previously the exact permissible ranges where given.

test/MC/Mips/msa/invalid.s
114–118

This change isn't consistent. The copy_u.h's are just out of range of the number of vector elements and given a specific error. The copy_u.w are also out of range and given a generic error.

sdardis accepted this revision.Dec 13 2017, 5:40 AM

Second look, this appears alright. We just need the near miss reporting facility to id the multiple errors.

test/MC/Mips/micromips32r6/invalid-wrong-error.s
9–16

If a line changed here you can move it from this test file to invalid.s in the same directory.

test/MC/Mips/target-soft-float.s
271

Restore this line as it is still correct.

274

Restore this line as it is still correct.

315

Restore this line as it is still correct.

326

Restore this line as it is still correct.

sdesmalen updated this revision to Diff 127125.Dec 15 2017, 7:10 AM

Restored the comments in target-soft-float.s and rebased to HEAD due to https://reviews.llvm.org/D40011.

test/MC/Mips/micromips32r6/invalid-wrong-error.s
9–16

I'm not sure I understand what you mean here.. ?

sdardis added inline comments.Dec 18 2017, 4:47 AM
test/MC/Mips/micromips32r6/invalid-wrong-error.s
9–16

The lines that changed here are now giving the correct error, so they're now in the wrong test file, as this file records instructions give a wrong error.

Copy these lines into test/MC/Mips/micromips32r6/invalid.s which tests that we give the correct error, and remove them from this file.

sdesmalen updated this revision to Diff 127346.Dec 18 2017, 6:21 AM

Moved some test lines to different file.

sdesmalen marked an inline comment as done.Dec 18 2017, 6:24 AM

Thanks @sdardis for checking the Mips tests. I've moved the test lines to the other file as you suggested.

sdesmalen closed this revision.Dec 18 2017, 6:35 AM