One major comment inlined, I'd like one of the other reviewers to chime in if it's the better solution.

A quick comment on the error message, inlined. It's about the quality of the diagnostics.

Looks like I missed some instructions. I'll update this patch shortly.

I think we need to modify LegalizeDAG .cpp here directly to eliminate the multiplication, as the legalizer has legalized the ISD::BR_JT into an address synthesis, load and multiplication. The legalizer then recursively legalizes the multiplication into a mult/mflo pair.

Can you re-upload this with full context?

LGTM.

Only some minor nits now, inlined.

Thanks for the review.

In D45020#1065586, @atanasyan wrote:

In D45020#1056816, @sdardis wrote:

we should use the 'mem' operand for the lb and lbu instructions. That will allow the parser to match larger offsets than those instructions permit.

BTW do you suggest to escape sub-classing the 'mem' operand at all? If so, we won't be able to diagnose that an operand does not fit 32 or 64 bit range.

Ok. The translation is covered by the irtranslator tests, so we should catch any unexpected changes.

The LLVM-IR in the MIR test files doesn't match the MIR below, but I haven't checked if that makes a difference.

LGTM.

You should also update the description of the patch.

I don't think this is quite the correct approach, especially if you're going to be implementing support for 64-bit pointers with those instructions, as we would need to modify the existing lb64 and friends to be available to the AsmParser with a different set of operands.

This looks mostly ok. The big changes required is that these instructions should go in the relevant base architecture .td, as I don't believe there's enough of them to warrant going into two separate files.

Thanks for the review.

Ping.

Ping.

Hi Milena, sorry for the delay in getting back to this, I', currently on holiday but I will get back to looking at this sometime next week.

LGTM, I'm not seeing an implementation of that method or a callee.

Some small nits inlined, I've mostly looked at the non debug related parts. A lot of my comments are the MIPS .td parts can be summarized as: where possible, modify the underlying *_DESC or MXC1 classes where possible rather than wrapping the definition of the instruction with a 'isRegMove = 1'. There are some cases where it can't be done as the instruction description class is shared multiple other instructions which are not moves.

@RKSimon , I'm ok with this going in, I'm in process of updating our scheduler models to account for this but it may take me a few days.

Thanks, I wasn't sure who to add as a reviewer.

Ping?

Update to remove unrelated changes to trap instructions.

LGTM with inline nits addressed.

This patch looks mostly ok, but I think there are some small issues with it.

Update to account for splitting out one of the changes into D44299.

Minor update, incorrect predicates for (s|l)w(l|r)e, fixed the ASE_EVA class overwriting the InsnPredicates instead of the ASEPredicates.

If you look at the dependency, you'll see the way I've separated out the ISA level from the ASEs.

Based on my comments on D44176, I'm preparing another patch which is the splitting out of the ASEPredicate from InsnPredicates in PredicateControl.

Ping.

This looks mostly ok. There's only some small changes required, and they are somewhat minor. The recurring change is that for test cases, when there is a run-on line with '\', then the continuation of the command line should be indented by two spaces.

Some comments on the MIPS part of this patch inlined.