[llvm] r279569 - GlobalISel: add forgotten test-case for G_ICMP

[Quentin Colombet via llvm-commits]

Hi Tim,

I missed when you added the type of the operand for the comparison and now, I have a question.

> On Aug 23, 2016, at 2:11 PM, Tim Northover via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Author: tnorthover
> Date: Tue Aug 23 16:11:36 2016
> New Revision: 279569
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=279569&view=rev
> Log:
> GlobalISel: add forgotten test-case for G_ICMP
> 
> Added:
>    llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-cmp.mir
> 
> Added: llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-cmp.mir
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-cmp.mir?rev=279569&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-cmp.mir (added)
> +++ llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-cmp.mir Tue Aug 23 16:11:36 2016
> @@ -0,0 +1,45 @@
> +# RUN: llc -O0 -run-pass=legalize-mir -global-isel %s -o - 2>&1 | FileCheck %s
> +
> +--- |
> +  target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
> +  target triple = "aarch64-apple-ios"
> +  define void @test_icmp() {
> +  entry:
> +    ret void
> +  }
> +...
> +
> +---
> +name:            test_icmp
> +isSSA:           true
> +registers:
> +  - { id: 0, class: _ }
> +  - { id: 1, class: _ }
> +  - { id: 2, class: _ }
> +  - { id: 3, class: _ }
> +body: |
> +  bb.0.entry:
> +    liveins: %x0, %x1, %x2, %x3
> +    %0(64) = COPY %x0
> +    %1(64) = COPY %x1
> +
> +    ; CHECK: [[TST32:%[0-9]+]](32) = G_ICMP { s32, s64 } intpred(sge), %0, %1
> +    ; CHECK: %2(1) = G_TRUNC { s1, s32 } [[TST32]]
> +    %2(1) = G_ICMP { s1, s64 } intpred(sge), %0, %1

Based on their definition here %0 and %1 are s64. 

> +
> +    ; CHECK: [[LHS32:%[0-9]+]](32) = G_ZEXT { s32, s8 } %0
> +    ; CHECK: [[RHS32:%[0-9]+]](32) = G_ZEXT { s32, s8 } %1
> +    ; CHECK: %3(32) = G_ICMP { s32, s32 } intpred(ne), [[LHS32]], [[RHS32]]
> +    %3(32) = G_ICMP { s32, s8 } intpred(ne), %0, %1

Here however, %0 and %1 becomes s8. I don’t think we want to allow to silently change the wide of a register. I mean that fine after selection but before it seems confusing.

Thus, I would have expected something like %2 = G_TRUNC %0 feeding the comparison.
The other reason why I am bringing that up is because I would have expected comparison to have a second type. I.e., I was expecting that we would have just look at the type of the definition to know that. Like we do for LLVM IR basically. Remains the question of the definition of target specific instruction/copies.

What do you think?

Note: I can live with the two types on the comparison, I believe that adds value for the verifier, but I don’t like the uses with different wides. Indeed, I think it is error prone to not have all “transitions” explicitly represents.
Note: That may just be a machine verifier check missing and you actually did not make that assumptions :).

Cheers,
-Quentin

> +
> +    ; CHECK: [[LHS32:%[0-9]+]](32) = G_ZEXT { s32, s8 } %0
> +    ; CHECK: [[RHS32:%[0-9]+]](32) = G_ZEXT { s32, s8 } %1
> +    ; CHECK: %3(32) = G_ICMP { s32, s32 } intpred(ugt), [[LHS32]], [[RHS32]]
> +    %3(32) = G_ICMP { s32, s8 } intpred(ugt), %0, %1
> +
> +    ; CHECK: [[LHS32:%[0-9]+]](32) = G_SEXT { s32, s8 } %0
> +    ; CHECK: [[RHS32:%[0-9]+]](32) = G_SEXT { s32, s8 } %1
> +    ; CHECK: %3(32) = G_ICMP { s32, s32 } intpred(sle), [[LHS32]], [[RHS32]]
> +    %3(32) = G_ICMP { s32, s8 } intpred(sle), %0, %1
> +
> +...
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits