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

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan 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) &&
----------------
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?


================
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));
----------------
- `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`


================
Comment at: lib/Target/Mips/MipsSEISelDAGToDAG.cpp:938
+      return true;
+    } else if (Node->getValueType(0) == MVT::i64) {
+      unsigned Opcode = 0;
----------------
Else-after-return https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


Repository:
  rL LLVM

https://reviews.llvm.org/D39117