[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:927
+
+    uint64_t Pos = Node->getConstantOperandVal(1);
+    uint64_t Size = Node->getConstantOperandVal(2);
----------------
atanasyan wrote:
> 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;
> ```
I agree, I think this is clearer.


https://reviews.llvm.org/D39117