[PATCH] D39117: [mips] Match 'ins' and its' variants with C++ code

[Simon Dardis via Phabricator via llvm-commits]

sdardis added inline comments.


================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:933
+        CurDAG->getTargetConstant(Size, DL, MVT::i32), Node->getOperand(3)};
+
+    if (Node->getValueType(0) == MVT::i32 && (0 <= Pos && Pos < 32) &&
----------------
atanasyan wrote:
> 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?
If size is zero, then it doesn't match the specification for any of the cases. I've changed the logic accordingly.


================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:935
+    if (Node->getValueType(0) == MVT::i32 && (0 <= Pos && Pos < 32) &&
+        (0 < Size <= 32) && (Size + Pos <= 32)) {
+      ReplaceNode(Node, CurDAG->getMachineNode(Mips::INS, DL, MVT::i32, Ops));
----------------
atanasyan wrote:
> - `0 <= Pos && Pos < 32` can be changed to `Pos < 32` because the `Pos` is unsigned integer.
> - I think `0 < Size <= 32` should be changed to the `0 < Size && Size <= 32`
Thanks for spotting, I've reflowed the logic around these checks.


https://reviews.llvm.org/D39117