This is an archive of the discontinued LLVM Phabricator instance.

Differential D5155

Fix PR18364: Only emit movw for ARM architectures that support it
ClosedPublic

Authored by dim on Sep 2 2014, 11:21 AM.

Details

Reviewers
rengolin
t.p.northover
Summary

See http://llvm.org/PR18364 for the initial report and discussion.

The fix is to add a HasV6T2 check to the ARMadde GPR:$src, imm0_65535_neg pattern, and add a testcase.

The only additional problem is that test/CodeGen/ARM/carry.ll fails after this change, because that test does not seem to emit a movw anymore. This may well have to do with the default ARM CPU chosen on a certain host system. The solution is probably to add a more explicit architecture in the llc command for this test. Suggestions welcome.

Diff Detail

Event Timeline

dim updated this revision to Diff 13174.Sep 2 2014, 11:21 AM
dim retitled this revision from to Fix PR18364: Only emit movw for ARM architectures that support it.
dim updated this object.
dim edited the test plan for this revision. (Show Details)
dim added reviewers: rengolin, t.p.northover.
Herald added a subscriber: aemerson. · View Herald TranscriptSep 2 2014, 11:21 AM
dim added a subscriber: Unknown Object (MLST).Sep 2 2014, 11:24 AM
rengolin edited edge metadata.Sep 2 2014, 11:27 AM

I agree you should add a more specific target for the failing test.

cheers,
--renato

test/CodeGen/ARM/pr18364-movw.ll
3

a bit pedantic, but can you add armv6t2? just to clear the boundaries

dim updated this revision to Diff 13175.Sep 2 2014, 11:46 AM
dim edited edge metadata.

Per Renato's suggestions:

  • Use armv6t2-eabi for test/CodeGen/ARM/carry.ll, so it may emit movw
  • Add explicit armv6t2 test to test/CodeGen/ARM/pr18364-movw.ll
rengolin accepted this revision.Sep 2 2014, 12:30 PM
rengolin edited edge metadata.

LGTM, thanks!

--renato

This revision is now accepted and ready to land.Sep 2 2014, 12:30 PM
dim added a comment.Sep 2 2014, 1:40 PM

It would be nice if somebody could commit it for me, as I don't have commit privileges. :)

rengolin closed this revision.Sep 2 2014, 3:56 PM

Committed in r216989 and r216990 (I missed the test).

cheers,
--renato

Revision Contents

PathSize
lib/
Target/
ARM/
3 lines
test/
CodeGen/
ARM/
2 lines
34 lines

Diff 13175

lib/Target/ARM/ARMInstrInfo.td

Show First 20 Lines • Show All 3,420 Lines • ▼ Show 20 Linesdef : ARMPat<(ARMaddc GPR:$src, imm0_65535_neg:$imm),
Requires<[IsARM, HasV6T2]>; Requires<[IsARM, HasV6T2]>;
// The with-carry-in form matches bitwise not instead of the negation. // The with-carry-in form matches bitwise not instead of the negation.
// Effectively, the inverse interpretation of the carry flag already accounts // Effectively, the inverse interpretation of the carry flag already accounts
// for part of the negation. // for part of the negation.
def : ARMPat<(ARMadde GPR:$src, so_imm_not:$imm, CPSR), def : ARMPat<(ARMadde GPR:$src, so_imm_not:$imm, CPSR),
(SBCri GPR:$src, so_imm_not:$imm)>; (SBCri GPR:$src, so_imm_not:$imm)>;
def : ARMPat<(ARMadde GPR:$src, imm0_65535_neg:$imm, CPSR), def : ARMPat<(ARMadde GPR:$src, imm0_65535_neg:$imm, CPSR),
(SBCrr GPR:$src, (MOVi16 (imm_not_XFORM imm:$imm)))>; (SBCrr GPR:$src, (MOVi16 (imm_not_XFORM imm:$imm)))>,
Requires<[IsARM, HasV6T2]>;
// Note: These are implemented in C++ code, because they have to generate // Note: These are implemented in C++ code, because they have to generate
// ADD/SUBrs instructions, which use a complex pattern that a xform function // ADD/SUBrs instructions, which use a complex pattern that a xform function
// cannot produce. // cannot produce.
// (mul X, 2^n+1) -> (add (X << n), X) // (mul X, 2^n+1) -> (add (X << n), X)
// (mul X, 2^n-1) -> (rsb X, (X << n)) // (mul X, 2^n-1) -> (rsb X, (X << n))
// ARM Arithmetic Instruction // ARM Arithmetic Instruction
▲ Show 20 Lines • Show All 2,203 Lines • Show Last 20 Lines

test/CodeGen/ARM/carry.ll

; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s ; RUN: llc -mtriple=armv6t2-eabi %s -o - | FileCheck %s
define i64 @f1(i64 %a, i64 %b) { define i64 @f1(i64 %a, i64 %b) {
; CHECK-LABEL: f1: ; CHECK-LABEL: f1:
; CHECK: subs r ; CHECK: subs r
; CHECK: sbc r ; CHECK: sbc r
entry: entry:
%tmp = sub i64 %a, %b %tmp = sub i64 %a, %b
ret i64 %tmp ret i64 %tmp
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines

test/CodeGen/ARM/pr18364-movw.ll

  • This file was added.
; RUN: llc < %s -mtriple=armv5te | FileCheck %s --check-prefix=V5
; RUN: llc < %s -mtriple=armv6 | FileCheck %s --check-prefix=V6
; RUN: llc < %s -mtriple=armv6t2 | FileCheck %s --check-prefix=V6T2
rengolinUnsubmitted
Not Done
ReplyInline Actions

a bit pedantic, but can you add armv6t2? just to clear the boundaries

rengolin: a bit pedantic, but can you add armv6t2? just to clear the boundaries
; RUN: llc < %s -mtriple=armv7 | FileCheck %s --check-prefix=V7
; PR18364
define i64 @f() #0 {
entry:
; V5-NOT: movw
; V6-NOT: movw
; V6T2: movw
; V7: movw
%y = alloca i64, align 8
%z = alloca i64, align 8
store i64 1, i64* %y, align 8
store i64 11579764786944, i64* %z, align 8
%0 = load i64* %y, align 8
%1 = load i64* %z, align 8
%sub = sub i64 %0, %1
ret i64 %sub
}
define i64 @g(i64 %a, i32 %b) #0 {
entry:
; V5-NOT: movw
; V6-NOT: movw
; V6T2: movw
; V7: movw
%0 = mul i64 %a, 86400000
%mul = add i64 %0, -210866803200000
%conv = sext i32 %b to i64
%add = add nsw i64 %mul, %conv
ret i64 %add
}