This is an archive of the discontinued LLVM Phabricator instance.

Differential D13857

Fix mapping of @llvm.arm.ssat/usat intrinsics to ssat/usat instructions
ClosedPublic

Authored by rmaprath on Oct 19 2015, 2:14 AM.

Details

Reviewers
rengolin
jmolloy
olista01
Summary

The mapping of these two intrinsics in ARMInstrInfo.td had a small omission which lead to their operands not being validated/transformed before being lowered into usat and ssat instructions. This can cause incorrect instructions to be emitted.

I've also added tests for the remaining two saturating arithmetic intrinsics @llvm.arm.qadd and @llvm.arm.qsub as they are missing codegen tests.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 37727.Oct 19 2015, 2:14 AM
rmaprath retitled this revision from to Fix mapping of @llvm.arm.ssat/usat intrinsics to ssat/usat instructions.
rmaprath updated this object.
rmaprath added reviewers: jmolloy, olista01.
rmaprath set the repository for this revision to rL LLVM.
rmaprath added a subscriber: llvm-commits.
Herald added subscribers: rengolin, aemerson. · View Herald TranscriptOct 19 2015, 2:14 AM
rengolin added a comment.Oct 19 2015, 2:40 AM

Shouldn't the tests account for the corner cases and validation errors as well?

rmaprath added a comment.Oct 19 2015, 2:50 AM
In D13857#270028, @rengolin wrote:

Shouldn't the tests account for the corner cases and validation errors as well?

I did think about this, but couldn't find a way to add such tests. If I put out-of-rage arguments for the intrinsics, instruction selection aborts with:

Script:
--
/work/oss/git/build/./bin/llc -O1 -mtriple=armv6-none-none-eabi /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll -o - | /work/oss/git/build/./bin/FileCheck /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll
--
Exit Code: 2

Command Output (stderr):
--
LLVM ERROR: Cannot select: intrinsic %llvm.arm.ssat
FileCheck error: '-' is empty.

--

And that completely kills the test (no further tests will execute). Any suggestions?

Thanks.

rengolin added a comment.Oct 19 2015, 2:55 AM
In D13857#270037, @rmaprath wrote:

/work/oss/git/build/./bin/llc -O1 -mtriple=armv6-none-none-eabi /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll -o - | /work/oss/git/build/./bin/FileCheck /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll

You have to separate the good tests from the bad ones in different files. The good ones, you do just like the one you have (but expand to the limits of the ranges accepted). The bad ones, just replace:

; RUN: llc ...

for

; RUN: not llc ...

and you'll be able to use FileCheck to parse the error messages.

There are many tests in CodeGen that follow this pattern.

rmaprath added a comment.Oct 19 2015, 2:57 AM
In D13857#270039, @rengolin wrote:
In D13857#270037, @rmaprath wrote:

/work/oss/git/build/./bin/llc -O1 -mtriple=armv6-none-none-eabi /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll -o - | /work/oss/git/build/./bin/FileCheck /work/oss/git/src/test/CodeGen/ARM/sat-arith.ll

You have to separate the good tests from the bad ones in different files. The good ones, you do just like the one you have (but expand to the limits of the ranges accepted). The bad ones, just replace:

; RUN: llc ...

for

; RUN: not llc ...

and you'll be able to use FileCheck to parse the error messages.

There are many tests in CodeGen that follow this pattern.

Thanks! I'll add the tests.

rmaprath updated this revision to Diff 37732.Oct 19 2015, 3:39 AM
rmaprath removed rL LLVM as the repository for this revision.

Updated patch to test the corner cases of ssat/usat intrinsics. qadd/qsub are not affected as they don't take in an immediate with range restrictions.

rengolin accepted this revision.Oct 19 2015, 3:49 AM
rengolin added a reviewer: rengolin.

Looks good. Thanks!

This revision is now accepted and ready to land.Oct 19 2015, 3:49 AM
rmaprath closed this revision.Oct 19 2015, 4:47 AM

Thanks. Committed as r250697.

Revision Contents

PathSize
lib/
Target/
ARM/
8 lines
test/
CodeGen/
ARM/
60 lines
10 lines
10 lines
10 lines
10 lines

Diff 37732

lib/Target/ARM/ARMInstrInfo.td

Context not available.
let Inst{3-0} = Rn; let Inst{3-0} = Rn;
} }
def : ARMV6Pat<(int_arm_ssat GPRnopc:$a, imm:$pos), def : ARMV6Pat<(int_arm_ssat GPRnopc:$a, imm1_32:$pos),
(SSAT imm:$pos, GPRnopc:$a, 0)>; (SSAT imm1_32:$pos, GPRnopc:$a, 0)>;
def : ARMV6Pat<(int_arm_usat GPRnopc:$a, imm:$pos), def : ARMV6Pat<(int_arm_usat GPRnopc:$a, imm0_31:$pos),
(USAT imm:$pos, GPRnopc:$a, 0)>; (USAT imm0_31:$pos, GPRnopc:$a, 0)>;
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// Bitwise Instructions. // Bitwise Instructions.
Context not available.

test/CodeGen/ARM/sat-arith.ll

  • This file was added.
; RUN: llc -O1 -mtriple=armv6-none-none-eabi %s -o - | FileCheck %s
; CHECK-LABEL: qadd
define i32 @qadd() nounwind {
; CHECK: mov [[R0:.*]], #8
; CHECK: mov [[R1:.*]], #128
; CHECK: qadd [[R0]], [[R1]], [[R0]]
%tmp = call i32 @llvm.arm.qadd(i32 128, i32 8)
ret i32 %tmp
}
; CHECK-LABEL: qsub
define i32 @qsub() nounwind {
; CHECK: mov [[R0:.*]], #8
; CHECK: mov [[R1:.*]], #128
; CHECK: qsub [[R0]], [[R1]], [[R0]]
%tmp = call i32 @llvm.arm.qsub(i32 128, i32 8)
ret i32 %tmp
}
; upper-bound of the immediate argument
; CHECK-LABEL: ssat1
define i32 @ssat1() nounwind {
; CHECK: mov [[R0:.*]], #128
; CHECK: ssat [[R1:.*]], #32, [[R0]]
%tmp = call i32 @llvm.arm.ssat(i32 128, i32 32)
ret i32 %tmp
}
; lower-bound of the immediate argument
; CHECK-LABEL: ssat2
define i32 @ssat2() nounwind {
; CHECK: mov [[R0:.*]], #128
; CHECK: ssat [[R1:.*]], #1, [[R0]]
%tmp = call i32 @llvm.arm.ssat(i32 128, i32 1)
ret i32 %tmp
}
; upper-bound of the immediate argument
; CHECK-LABEL: usat1
define i32 @usat1() nounwind {
; CHECK: mov [[R0:.*]], #128
; CHECK: usat [[R1:.*]], #31, [[R0]]
%tmp = call i32 @llvm.arm.usat(i32 128, i32 31)
ret i32 %tmp
}
; lower-bound of the immediate argument
; CHECK-LABEL: usat2
define i32 @usat2() nounwind {
; CHECK: mov [[R0:.*]], #128
; CHECK: usat [[R1:.*]], #0, [[R0]]
%tmp = call i32 @llvm.arm.usat(i32 128, i32 0)
ret i32 %tmp
}
declare i32 @llvm.arm.qadd(i32, i32) nounwind
declare i32 @llvm.arm.qsub(i32, i32) nounwind
declare i32 @llvm.arm.ssat(i32, i32) nounwind readnone
declare i32 @llvm.arm.usat(i32, i32) nounwind readnone

test/CodeGen/ARM/ssat-lower.ll

  • This file was added.
; RUN: not llc < %s -O1 -mtriple=armv6-none-none-eabi 2>&1 | FileCheck %s
; immediate argument < lower-bound
; CHECK: LLVM ERROR: Cannot select: intrinsic %llvm.arm.ssat
define i32 @ssat1() nounwind {
%tmp = call i32 @llvm.arm.ssat(i32 128, i32 0)
ret i32 %tmp
}
declare i32 @llvm.arm.ssat(i32, i32) nounwind readnone

test/CodeGen/ARM/ssat-upper.ll

  • This file was added.
; RUN: not llc < %s -O1 -mtriple=armv6-none-none-eabi 2>&1 | FileCheck %s
; immediate argument > upper-bound
; CHECK: LLVM ERROR: Cannot select: intrinsic %llvm.arm.ssat
define i32 @ssat1() nounwind {
%tmp = call i32 @llvm.arm.ssat(i32 128, i32 33)
ret i32 %tmp
}
declare i32 @llvm.arm.ssat(i32, i32) nounwind readnone

test/CodeGen/ARM/usat-lower.ll

  • This file was added.
; RUN: not llc < %s -O1 -mtriple=armv6-none-none-eabi 2>&1 | FileCheck %s
; immediate argument < lower-bound
; CHECK: LLVM ERROR: Cannot select: intrinsic %llvm.arm.usat
define i32 @usat1() nounwind {
%tmp = call i32 @llvm.arm.usat(i32 128, i32 -1)
ret i32 %tmp
}
declare i32 @llvm.arm.usat(i32, i32) nounwind readnone

test/CodeGen/ARM/usat-upper.ll

  • This file was added.
; RUN: not llc < %s -O1 -mtriple=armv6-none-none-eabi 2>&1 | FileCheck %s
; immediate argument > upper-bound
; CHECK: LLVM ERROR: Cannot select: intrinsic %llvm.arm.usat
define i32 @usat1() nounwind {
%tmp = call i32 @llvm.arm.usat(i32 128, i32 32)
ret i32 %tmp
}
declare i32 @llvm.arm.usat(i32, i32) nounwind readnone