[PATCH] D48374: [mips] Sign extend i32 return values on MIPS64

[Stefan Maksimovic via Phabricator via llvm-commits]

smaksimovic requested review of this revision.
smaksimovic added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:2140
+  /// combined in certain conditions.
+  virtual bool keepSextInRegNode(EVT VT) const {
+    return false;
----------------
efriedma wrote:
> Is this required for correctness?  If it is, then the MIPS target is lying to DAGCombine somehow, and this fix is just papering over the bug.  Otherwise, I can't see how keeping around a redundant SIGN_EXTEND_INREG would make the generated code more efficient.
The sole purpose of that function, and keeping the SIGN_EXTEND_INREG node alive, is so that it could be later matched with this pattern:

```
def : MipsPat<(i64 (sext_inreg GPR64:$src, i32)),
              (SLL64_64 GPR64:$src)>;
```
As an example, SelectionDAG for the or_i32 function which resides in the or.ll test file:

```
  t0: ch = EntryToken
            t2: i64,ch = CopyFromReg t0, Register:i64 %0
          t4: i64 = AssertSext t2, ValueType:ch:i32
        t5: i32 = truncate t4
            t7: i64,ch = CopyFromReg t0, Register:i64 %1
          t8: i64 = AssertSext t7, ValueType:ch:i32
        t9: i32 = truncate t8
      t10: i32 = or t5, t9
    t11: i64 = sign_extend t10
  t13: ch,glue = CopyToReg t0, Register:i64 $v0_64, t11
  t14: ch = MipsISD::Ret t13, Register:i64 $v0_64, t13:1
```
gets eventually transformed to this (including the sign_extend_inreg node incorporated by this patch):
```
  t0: ch = EntryToken
          t2: i64,ch = CopyFromReg t0, Register:i64 %0
        t4: i64 = AssertSext t2, ValueType:ch:i32
          t7: i64,ch = CopyFromReg t0, Register:i64 %1
        t8: i64 = AssertSext t7, ValueType:ch:i32
      t15: i64 = or t4, t8
    t17: i64 = sign_extend_inreg t15, ValueType:ch:i32
  t13: ch,glue = CopyToReg t0, Register:i64 $v0_64, t17
  t14: ch = MipsISD::Ret t13, Register:i64 $v0_64, t13:1
```
In place of sign_extend_inreg node would be the sign_extend node, which would be removed the same way the sign_extend_inreg would.
Hence the keepSextInRegNode function to keep it until it can be pattern matched and replaced with the target dependent machine node.
I completely agree that its existence is indeed questionable, looking at the code above by itself.
However I have not found a better way to go about this change.


https://reviews.llvm.org/D48374