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

[Stefan Maksimovic via Phabricator via llvm-commits]

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:
> 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?
Its purpose in this case is to sign extend the return value of a function, specifically i32 on MIPS64. At the moment that is not the case, even if the i32 return value is marked with the signext attribute.
On MIPS, only the arithmetic instructions implicitly sign extend their results so we need to keep the sll instruction which does the extend for other operations prior to returning.


https://reviews.llvm.org/D48374