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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:2140
+  /// combined in certain conditions.
+  virtual bool keepSextInRegNode(EVT VT) const {
+    return false;
----------------
smaksimovic wrote:
> 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.
I don't understand: why do you want to generate a redundant "sll" instruction?


https://reviews.llvm.org/D48374