[PATCH] D20475: [mips] Enforce compact branch register restrictions
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Tue May 31 08:22:59 PDT 2016
dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.
LGTM with some nits and the new tests enabled.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3644-3659
@@ -3641,6 +3643,18 @@
unsigned MipsAsmParser::checkTargetMatchPredicate(MCInst &Inst) {
// As described by the Mips32r2 spec, the registers Rd and Rs for
// jalr.hb must be different.
// It also applies for registers Rt and Rs of microMIPSr6 jalrc.hb instruction
// and registers Rd and Base for microMIPS lwp instruction
+
+ // As described the MIPSR6 spec, the compact branches that compare registers
+ // must:
+ // a) Not use the zero register.
+ // b) Not use the same register twice.
+ // c) rs < rt for bnec, beqc.
+ // NB: For this case, LLVM's direct object emission will swap the operands
+ // as their ordering doesn't matter. GAS performs this transformation
+ // too. Hence, that constraint does not have to be enforced.
+ //
+ // The compact branches that branch iff the signed addition of two registers
+ // would overflow must have rs >= rt.
unsigned Opcode = Inst.getOpcode();
----------------
Can we put these comments nearer the relevant block of code?
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3654
@@ +3653,3 @@
+ // c) rs < rt for bnec, beqc.
+ // NB: For this case, LLVM's direct object emission will swap the operands
+ // as their ordering doesn't matter. GAS performs this transformation
----------------
LLVM's direct object emission
should be 'the encoding' since encoding for other purposes will swap the operands too.
================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3662-3683
@@ -3647,8 +3661,24 @@
if ((Opcode == Mips::JALR_HB || Opcode == Mips::JALRC_HB_MMR6) &&
(Inst.getOperand(0).getReg() == Inst.getOperand(1).getReg()))
return Match_RequiresDifferentSrcAndDst;
else if ((Opcode == Mips::LWP_MM || Opcode == Mips::LWP_MMR6) &&
(Inst.getOperand(0).getReg() == Inst.getOperand(2).getReg()))
return Match_RequiresDifferentSrcAndDst;
+ else if ((Opcode == Mips::BLEZC || Opcode == Mips::BGEZC ||
+ Opcode == Mips::BGEC || Opcode == Mips::BGTZC ||
+ Opcode == Mips::BLTZC || Opcode == Mips::BLTC ||
+ Opcode == Mips::BGEUC || Opcode == Mips::BLTUC ||
+ Opcode == Mips::BEQC || Opcode == Mips::BNEC ||
+ Opcode == Mips::BEQZC || Opcode == Mips::BNEZC) &&
+ (Inst.getOperand(0).getReg() == Mips::ZERO))
+ return Match_RequiresNoZeroRegister;
+ else if (Opcode == Mips::BGEC || Opcode == Mips::BLTC ||
+ Opcode == Mips::BGEUC || Opcode == Mips::BLTUC ||
+ Opcode == Mips::BEQC || Opcode == Mips::BNEC) {
+ if (Inst.getOperand(1).getReg() == Mips::ZERO)
+ return Match_RequiresNoZeroRegister;
+ if (Inst.getOperand(0).getReg() == Inst.getOperand(1).getReg())
+ return Match_RequiresDifferentOperands;
+ }
----------------
Now that this is starting to grow I think we ought to change the way we write this.
I think we ought to convert it to a switch statement:
switch(Opcode) {
case Mips::JALR_HB:
case Mips::JALRC_HB_MMR6:
if (Inst.getOperand(0).getReg() == Inst.getOperand(1).getReg()))
return Match_RequiresDifferentSrcAndDst;
return Match_Success;
case Mips::LWP_MM:
case Mips::LWP_MMR6:
if (Inst.getOperand(0).getReg() == Inst.getOperand(2).getReg()))
return Match_RequiresDifferentSrcAndDst;
return Match_Success;
...
default:
return Match_Success;
}
================
Comment at: test/MC/Mips/mips32r6/invalid.s:39
@@ -38,3 +38,3 @@
lhue $4, 512($2) # CHECK: :[[@LINE]]:{{[0-9]+}}: error: expected memory with 9-bit signed offset
// FIXME: Following tests are temporarely disabled, until "PredicateControl not in hierarchy" problem is resolved
bltl $7, $8, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: instruction requires a CPU feature not currently enabled
----------------
Could you fix the typo on `temporarely` while you're here?
================
Comment at: test/MC/Mips/mips32r6/invalid.s:48-65
@@ -47,2 +47,20 @@
bgtul $7, $8, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: instruction requires a CPU feature not currently enabled
+ bgec $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltc $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgeuc $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltuc $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ beqc $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnec $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgec $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgeuc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltuc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ beqc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnec $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ blezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgtzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ beqzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
cache -1, 255($7) # CHECK: :[[@LINE]]:15: error: expected 5-bit unsigned immediate
----------------
Did you mean to disable these new tests?
================
Comment at: test/MC/Mips/mips64r6/invalid.s:48-61
@@ -47,2 +47,16 @@
bgtul $7, $8, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: instruction requires a CPU feature not currently enabled
+ beqc $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnec $0, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgec $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgeuc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltuc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ beqc $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnec $2, $2, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ blezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bgtzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bltzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ beqzc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
+ bnezc $0, local_label # -CHECK: :[[@LINE]]:{{[0-9]+}}: error: invalid operand ($zero) for instruction
cache -1, 255($7) # CHECK: :[[@LINE]]:15: error: expected 5-bit unsigned immediate
----------------
Did you mean to disable these new tests?
Repository:
rL LLVM
http://reviews.llvm.org/D20475
More information about the llvm-commits
mailing list