This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Asm: Improved diagnostics when a memory operand and unsupported CPU feature are involved
Needs ReviewPublic

Authored by Tazdevil971 on Feb 6 2023, 7:58 AM.

Details

Reviewers
lattner
atanasyan
Summary

This improves diagnostic by using the same changes set up to fix the same problem on AArch64, see D40363.

This applies to all instructions that contain a memory operand and are currently not disabled via some feature flag.

Diff Detail

Event Timeline

Tazdevil971 created this revision.Feb 6 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 7:58 AM
Tazdevil971 requested review of this revision.Feb 6 2023, 7:58 AM

I'm sorry but I'm too far out of this code to review it.

Don't worry, sorry to have pinged the wrong person. This is my first review here and I'm having troubles finding a reviewer. I think I'm going to ask on discourse. Thank you for your time!

This makes sense to me, though a lot of targets leave it at its default false value still. Is there a situation where you wouldn't want this?

llvm/test/MC/Mips/cnmips/invalid.s
28

Please fix these

Yes and no, not all targets benefit from this the same way. Also it doesn't always work as intended, if you look at file llvm/test/MC/Mips/mips2/invalid-mips3-wrong-error.s, there is a case where it actually gives you worst diagnostics.

It all depends on the target and how good the fallback parser is, in the case of MIPS it's pretty bad, as it only handles immediates and registers (thus the old "wrong operand" error, the fallback parser was incorrectly guessing the kind of operand, immediate instead of memory). As far as I understand the issue was similar with AArch64 as SVE added a lot of complexity to correctly guessing the kind of operand so they resorted to this solution.

I think it's more of a case by case thing, and we should review each case individually.

llvm/test/MC/Mips/cnmips/invalid.s
28

Sorry, what's wrong about them? These are COP3 instructions which are disabled when cnmips is enabled, the message is kind of correct, these instructions require a different set of CPU features.

Or do you mean the file endings?

Fixed "no newlines"

llvm/test/MC/Mips/mips1/invalid-mips2.s