Page MenuHomePhabricator

[mips] Implement jr.hb and jalr.hb (Jump Register and Jump and Link Register with Hazard Barrier).
ClosedPublic

Authored by matheusalmeida on Jun 4 2014, 7:57 AM.

Details

Summary

These instructions are available in ISAs >= mips32/mips64. For mips32r6/mips64r6, jr.hb has a new encoding format.

Diff Detail

Event Timeline

matheusalmeida retitled this revision from to [mips] Implement jr.hb and jalr.hb..
matheusalmeida updated this object.
matheusalmeida edited the test plan for this revision. (Show Details)

Remove blank line.

Removes isBranch, isTerminator and isBarrier from JALR.

Accidentally removed isTerminator and isBranch from JR_HB.

Fields isTerminator and isBarrier should be defined in JR_HB_R6_DESC and not
overriden using let (copy and paste error).

dsanders edited edge metadata.Jun 9 2014, 3:15 AM

Mostly nits but there's a probable functionality bug too.

lib/Target/Mips/Mips32r6InstrInfo.td
561

Please rename the def to JR_HB_R6 so that it matches the naming scheme.

Also, shouldn't JR_HB_ENC be JR_HB_R6_ENC?

lib/Target/Mips/MipsInstrInfo.td
244–247

The other *_NOT_* adjectives immediately follow the one without the 'NOT'. Could you move it up to line 235?

1285–1312

In the long term, I'm in favour of moving to this style and naming convention throughout the MIPS tablegen files. However, I've been trying to stay consistent within this file for now since we will be able to identify old-style definitions by their name. I think it would be better to rename these to match the style used in the rest of this file.

1314–1315

Please add the underscores to the def name.

1353

Shouldn't this be a ISA_MIPS32_NOT_32R6_64R6?

test/MC/Mips/mips32r2/invalid.s
10–11

Just for consistency with the other files, could you put the ASM: on the same line as the instruction? Also one nice side effect of doing this is it allows sorting to maintain the correct order.

Same for the other invalid.s's.

matheusalmeida added inline comments.Jun 9 2014, 5:14 AM
lib/Target/Mips/Mips32r6InstrInfo.td
561

Also, shouldn't JR_HB_ENC be JR_HB_R6_ENC?

Are you suggesting I should rename JR_H_ENC to JR_HB_R6_ENC or are you actually asking if JR_HB_ENC is the right encoding format ? JR_HB_ENC is JR_HB_R6_FM<OPCODE6_JALR> and the old encoding (mips32) doesn't use the _ENC naming scheme (because MipsInstrInfo.td doesn't use it).

lib/Target/Mips/MipsInstrInfo.td
1353

jalr.hb didn't change between mips32 and mips32r6 which means this alias remains valid to 32R6 and 64R6.

dsanders added inline comments.Jun 9 2014, 5:41 AM
lib/Target/Mips/Mips32r6InstrInfo.td
561

Just the name. I think there's potential for confusion if we use an _R6 suffix on the def and the *_DESC, but don't use it on the *_ENC as well.

lib/Target/Mips/MipsInstrInfo.td
1353

You're right, there is no 32R6/64R6-specific jalr.hb. Sorry.

matheusalmeida edited edge metadata.
  1. Moved check CHECK lines to the same line as the instruction it's testing
  2. Updated names (added underscores and _R6 when needed)
  3. Moved definition of ISA_MIPS32_NOT_32R6_64R6 closer to definition of ISA_MIPS32 as requested
dsanders accepted this revision.Jun 11 2014, 5:21 AM
dsanders edited edge metadata.

LGTM. Thanks

This revision is now accepted and ready to land.Jun 11 2014, 5:21 AM
matheusalmeida retitled this revision from [mips] Implement jr.hb and jalr.hb. to [mips] Implement jr.hb and jalr.hb (Jump Register and Jump and Link Register with Hazard Barrier)..Jun 11 2014, 7:47 AM
matheusalmeida updated this object.
matheusalmeida edited edge metadata.

I've just noticed that some of the invalid.s's weren't updated to put the ASM on the same line as the instruction.
Could you fix that?

Done in r210757.