Change the ISel matching of 'ins', 'dins[mu]' from tablegen code to
C++ code. This resolves an issue where ISel would select 'dins' instead
of 'dinsm' when the instructions size and position were individually in
range but their sum was out of range according to the ISA specification.
Details
Details
Diff Detail
Diff Detail
- Build Status
Buildable 11425 Build 11425: arc lint + arc unit
Event Timeline
| lib/Target/Mips/MipsSEISelDAGToDAG.cpp | ||
|---|---|---|
| 933 | In the code below if Pos is greater than zero and Size is 0 and Node->getValueType(0) is equal to MVT::i32, the function returns false. But if Node->getValueType(0) is equal to MVT::i64, the function returns true and replaces the node by the Mips::DINS opcode. Does it conform the specification? | |
| 935 |
| |
| 938 | Else-after-return https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return | |
Comment Actions
Address review comments.
I've reflowed the logic for handling dins* to match the ISA documentation, with the exception of an early return if the size is zero.
| lib/Target/Mips/MipsSEISelDAGToDAG.cpp | ||
|---|---|---|
| 933 | If size is zero, then it doesn't match the specification for any of the cases. I've changed the logic accordingly. | |
| 935 | Thanks for spotting, I've reflowed the logic around these checks. | |
| lib/Target/Mips/MipsSEISelDAGToDAG.cpp | ||
|---|---|---|
| 927 | Now the code below is okay, but i think we can write it a bit more clear. I think the following variant is logically equal to the original one. But I do not force to use it. uint64_t MVT = Node->getConstantOperandVal(0);
uint64_t Pos = Node->getConstantOperandVal(1);
uint64_t Size = Node->getConstantOperandVal(2);
// Size has to be >0 for 'ins', 'dins' and 'dinsu'.
if (!Size)
return false;
if (Size + Pos > 64)
return false;
unsigned Opcode = 0;
if (MVT == MVT::i32) {
if (Pos + Size <= 32)
Opcode = Mips::INS;
} else {
if (Pos + Size <= 32)
Opcode = Mips::DINS;
else if (Pos < 32)
Opcode = Mips::DINSM;
else
Opcode = Mips::DINSU;
}
if (Opcode) {
SDValue Ops[4] = {
MVT, CurDAG->getTargetConstant(Pos, DL, MVT::i32),
CurDAG->getTargetConstant(Size, DL, MVT::i32), Node->getOperand(3)};
ReplaceNode(Node, CurDAG->getMachineNode(Opcode, DL, MVT, Ops));
return true;
}
return false; | |
| lib/Target/Mips/MipsSEISelDAGToDAG.cpp | ||
|---|---|---|
| 927 | I agree, I think this is clearer. | |
Now the code below is okay, but i think we can write it a bit more clear. I think the following variant is logically equal to the original one. But I do not force to use it.
uint64_t MVT = Node->getConstantOperandVal(0); uint64_t Pos = Node->getConstantOperandVal(1); uint64_t Size = Node->getConstantOperandVal(2); // Size has to be >0 for 'ins', 'dins' and 'dinsu'. if (!Size) return false; if (Size + Pos > 64) return false; unsigned Opcode = 0; if (MVT == MVT::i32) { if (Pos + Size <= 32) Opcode = Mips::INS; } else { if (Pos + Size <= 32) Opcode = Mips::DINS; else if (Pos < 32) Opcode = Mips::DINSM; else Opcode = Mips::DINSU; } if (Opcode) { SDValue Ops[4] = { MVT, CurDAG->getTargetConstant(Pos, DL, MVT::i32), CurDAG->getTargetConstant(Size, DL, MVT::i32), Node->getOperand(3)}; ReplaceNode(Node, CurDAG->getMachineNode(Opcode, DL, MVT, Ops)); return true; } return false;