[PATCH] D18893: [mips] Sign-extend i32 values truncated from previously zero-extended i32 values.

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:823-827
@@ +822,7 @@
+
+  // fold (AssertZext (trunc (AssertZext x) -> (trunc (AssertZext x))
+  // if the type of the extension of the innermost AssertZext node is
+  // smaller from that of the outermost node, eg:
+  // (AssertZext (trunc:i32 (AssertZext:i64 x, i8)), i32) ->
+  //                                                (trunc (AssertZext x, i8))
+  SDValue OuterAssertZext = N0.getOperand(0);
----------------
dsanders wrote:
> dsanders wrote:
> > Some closing parenthesis are missing on the LHS.
> The code implements the right behaviour but I think the expressions in the comment may be inside-out and the usage of 'outer' and 'inner' are the opposite way around to what I'd expect ('outer' is used for `N->getOperand(0)->getOperand(0)`). Shouldn't the first expression be?:
>   (AssertZext:i32 (trunc:i32 (AssertZext:i64 X, i32)), i8)
>     -> (trunc:i32 (AssertZext X, i8))
> 
> Also, I don't think the test cases cover this dag combine rule at the moment. assertzext-trunc.ll doesn't trigger the assertzext/trunc/assertzext case we've looked at before. divrem.ll and octeon_popcnt.ll used to trigger it but don't anymore because of the change to make the arguments signext.
> Shouldn't the first expression be?:
> 
> (AssertZext:i32 (trunc:i32 (AssertZext:i64 X, i32)), i8)
>   -> (trunc:i32 (AssertZext X, i8))

That's correct, I swapped the VTs accidentally.

> Also, I don't think the test cases cover this dag combine rule at the moment.

It's covered (albeit implicitly) by llvm-ir/{udiv,lshr}.ll and mips64-f128.ll. I copied the IR that triggers this behaviour from llvm-ir/udiv.ll to assertzext-trunc.ll.


http://reviews.llvm.org/D18893