This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix test CodeGen/ARM/fpcmp_ueq.ll broken by rL290616
ClosedPublic

Authored by eastig on Dec 29 2016, 8:27 AM.

Details

Summary

Commit rL290616 (https://reviews.llvm.org/rL290616) changed a checking command for the triple arm-apple-darwin in LLVM::CodeGen/ARM/fpcmp_ueq.ll from

RUN: llc < %s -mtriple=arm-apple-darwin | grep moveq

to

RUN: llc -mtriple arm-apple-darwin -filetype asm -o - %s | FileCheck -check-prefix CHECK-ARMv4 %s

It also added the following checks:

; CHECK-ARMv4-LABEL: f7:
; CHECK-ARMv4: moveq r6, #1
; CHECK-ARMv4: moveq r0, #42

These changes introduced a sequence of instructions to be checked. Before the changes it was expected any number of moveq. Now it is expected two concrete moveq.

The tested code is:

define i32 @f7(float %a, float %b) {
entry:
  %tmp = fcmp ueq float %a,%b
  %retval = select i1 %tmp, i32 666, i32 42
  ret i32 %retval
}

For the triple 'arm-apple-darwin' the generated code is:

        .section        __TEXT,__text,regular,pure_instructions
        .syntax unified
        .globl  _f7
        .p2align        2
        .code   32                      @ @f7
_f7:
@ BB#0:                                 @ %entry
        push    {r4, r5, r6, lr}
        mov     r4, r1
        mov     r5, r0
        bl      ___eqsf2
        cmp     r0, #0
        mov     r6, #0
        mov     r0, r5
        mov     r1, r4
        moveq   r6, #1
        bl      ___unordsf2
        cmp     r0, #0
        movne   r0, #1
        orrs    r0, r0, r6
        mov     r0, #154
        orr     r0, r0, #512
        moveq   r0, #42
        pop     {r4, r5, r6, lr}
        mov     pc, lr
 
.subsections_via_symbols

The final result depends on results of ___eqsf2 and ___unordsf2. 'r6' is used to keep a value based on a result of ___eqsf2. First, the name of the register is not fixed. Second, the value in 'r6' can be constructed without using moveq.

The patch introduces another sequence of instructions to be checked:

; CHECK-ARMv4-LABEL: f7:
; CHECK-ARMv4: bl
; CHECK-ARMv4: bl
; CHECK-ARMv4: cmp r0, #0
; CHECK-ARMv4: movne r0, #1
; CHECK-ARMv4: orrs r0, r0,
; CHECK-ARMv4: moveq r0, #42

It means there are calls of some functions then their results are combined and the final result in 'r0' is based on the combined result.

Diff Detail

Repository
rL LLVM

Event Timeline

eastig updated this revision to Diff 82671.Dec 29 2016, 8:27 AM
eastig retitled this revision from to [ARM] Fix test CodeGen/ARM/fpcmp_ueq.ll broken by rL290616.
eastig updated this object.
eastig added reviewers: rengolin, t.p.northover.
eastig added subscribers: llvm-commits, compnerd.
rengolin edited edge metadata.Jan 10 2017, 9:51 AM

The original test was really bad, the current state is better, but I agree, those check lines make no attempt to understand the test.

I think this patch is better in that respect, but I want to understand a bit more. Why do you omit the function names in the bl check? Do they change? Should they?

Hi Renato,
Thank for looking at the patch.
The main reason for omitting the function names is that they are specific to FP ABI of a system. ' ___eqsf2' and '___unordsf2' are gcc soft float library functions. I am not an expert in Darwin which is used in the triple. As Darwin is based on BSD we might assume the same functions are always used on it. Can we?
Another option is to add comments to the naked BLs explaining what can be there.

I prefer to have the function names in the checks.

Thanks,
Evgeny

The main reason for omitting the function names is that they are specific to FP ABI of a system. ' ___eqsf2' and '___unordsf2' are gcc soft float library functions. I am not an expert in Darwin which is used in the triple. As Darwin is based on BSD we might assume the same functions are always used on it. Can we?

Whatever LLVM outputs there should be correct for Apple. If they use the same functions, then you should write them down in the tests.

I think that the reason the names are not in there is that one doesn't depend on the other's arguments, so they could potentially come in different order.

One way to avoid that is to use CHECK-DAG just for the two bl calls, and keeping the rest as you wrote.

cheers,
--renato

eastig updated this revision to Diff 83972.Jan 11 2017, 6:48 AM
eastig edited edge metadata.

Updated the test according to Renato's comments.

rengolin accepted this revision.Jan 11 2017, 7:06 AM
rengolin edited edge metadata.

Thanks for the changes, LGTM.

This revision is now accepted and ready to land.Jan 11 2017, 7:06 AM
This revision was automatically updated to reflect the committed changes.