This is an archive of the discontinued LLVM Phabricator instance.

Differential D69099

[TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (Baseline Tests)
ClosedPublic

Authored by joanlluch on Oct 17 2019, 4:12 AM.

Details

Reviewers
spatel
lebedev.ri
Commits
rG0a15981a84b9: [MSP430] Shift Amount Threshold in DAGCombine (Baseline Tests); NFC
rL375345: [MSP430] Shift Amount Threshold in DAGCombine (Baseline Tests); NFC
Summary

This is baseline test assertions for before applying D68982

Diff Detail

Event Timeline

joanlluch created this revision.Oct 17 2019, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 4:12 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
spatel added inline comments.Oct 17 2019, 5:05 AM
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
3–4

I'm not sure if there's a difference, but I usually specify the triple in the RUN line, and I don't explicitly set a datalayout.

6

Here and below: remove unnecessary "dso_local"

160

Is it intentional that all tests use the i16 type? Would it be beneficial/increase test coverage to include other common types like i32/i64?

joanlluch marked 2 inline comments as done.Oct 17 2019, 5:58 AM
joanlluch added inline comments.
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
3–4

I think there's no difference. I just followed the pattern found in other tests of the same target such as the shifts.ll. Also, the form that I used is what clang generates, so I thought it would be more representative. But I can replace it if considered not suitable.

6

I'm unsure about what dsl_local means, this was generated by clang, but I'll remove it if not necessary.

160

Yes, the i16 type is intentional. The target does not natively support larger types, so larger types are expanded into operations with register pairs requiring additional transformations, or are custom lowered, which confuse the purposes of this patch.

joanlluch updated this revision to Diff 225424.Oct 17 2019, 7:08 AM

Updated baseline test file with the given recommendations:

  • removed explicit target and datalayout from .ll test file
  • removed dso_local
spatel accepted this revision.Oct 17 2019, 7:09 AM

LGTM

llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
6

For our needs within this file, I think it's just noise:
http://llvm.org/docs/LangRef.html#runtime-preemption-specifiers

This revision is now accepted and ready to land.Oct 17 2019, 7:09 AM
joanlluch planned changes to this revision.Oct 17 2019, 7:34 AM

@spatel
I just realised that I somehow messed this test file with the already optimised one. Please discard this until I upload the right one. Thanks

joanlluch updated this revision to Diff 225444.Oct 17 2019, 8:46 AM

Should be ok now.

This revision is now accepted and ready to land.Oct 17 2019, 8:46 AM
joanlluch mentioned this in D69116: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (1/2).Oct 17 2019, 9:51 AM
joanlluch mentioned this in D69120: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (2/2).Oct 17 2019, 10:35 AM
joanlluch mentioned this in D68982: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine.Oct 17 2019, 10:40 AM
Closed by commit rG0a15981a84b9: [MSP430] Shift Amount Threshold in DAGCombine (Baseline Tests); NFC (authored by spatel). · Explain WhyOct 19 2019, 9:28 AM
This revision was automatically updated to reflect the committed changes.
spatel mentioned this in rG113181e9bd05: [DAGCombine][MSP430] use shift amount threshold in DAGCombine (2/2).Nov 4 2019, 10:47 AM

Revision Contents

PathSize
llvm/
test/
CodeGen/
MSP430/
181 lines

Diff 225395

llvm/test/CodeGen/MSP430/shift-amount-threshold.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc < %s | FileCheck %s
target datalayout = "e-m:e-p:16:16-i32:16-i64:16-f32:16-f64:16-a:8-n8:16-S16"
target triple = "msp430"
spatelUnsubmitted
Not Done
ReplyInline Actions

I'm not sure if there's a difference, but I usually specify the triple in the RUN line, and I don't explicitly set a datalayout.

spatel: I'm not sure if there's a difference, but I usually specify the triple in the RUN line, and I…
joanlluchAuthorUnsubmitted
Done
ReplyInline Actions

I think there's no difference. I just followed the pattern found in other tests of the same target such as the shifts.ll. Also, the form that I used is what clang generates, so I thought it would be more representative. But I can replace it if considered not suitable.

joanlluch: I think there's no difference. I just followed the pattern found in other tests of the same…
define dso_local i16 @testSimplifySetCC_0(i16 %a) {
spatelUnsubmitted
Not Done
ReplyInline Actions

Here and below: remove unnecessary "dso_local"

spatel: Here and below: remove unnecessary "dso_local"
joanlluchAuthorUnsubmitted
Done
ReplyInline Actions

I'm unsure about what dsl_local means, this was generated by clang, but I'll remove it if not necessary.

joanlluch: I'm unsure about what dsl_local means, this was generated by clang, but I'll remove it if not…
spatelUnsubmitted
Not Done
ReplyInline Actions

For our needs within this file, I think it's just noise:
http://llvm.org/docs/LangRef.html#runtime-preemption-specifiers

spatel: For our needs within this file, I think it's just noise: http://llvm.org/docs/LangRef.
; CHECK-LABEL: testSimplifySetCC_0:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: and #32, r12
; CHECK-NEXT: clrc
; CHECK-NEXT: rrc r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: ret
entry:
%and = and i16 %a, 32
%cmp = icmp ne i16 %and, 0
%conv = zext i1 %cmp to i16
ret i16 %conv
}
define dso_local i16 @testSimplifySetCC_1(i16 %a) {
; CHECK-LABEL: testSimplifySetCC_1:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: and #32, r12
; CHECK-NEXT: clrc
; CHECK-NEXT: rrc r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: ret
entry:
%and = and i16 %a, 32
%cmp = icmp eq i16 %and, 32
%conv = zext i1 %cmp to i16
ret i16 %conv
}
define dso_local i16 @testSiymplifySelect(i16 %a) {
; CHECK-LABEL: testSiymplifySelect:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: mov r12, r13
; CHECK-NEXT: clr r12
; CHECK-NEXT: bit #2048, r13
; CHECK-NEXT: jeq .LBB2_2
; CHECK-NEXT: ; %bb.1: ; %entry
; CHECK-NEXT: mov #3, r12
; CHECK-NEXT: .LBB2_2: ; %entry
; CHECK-NEXT: ret
entry:
%and = and i16 %a, 2048
%cmp = icmp eq i16 %and, 0
%cond = select i1 %cmp, i16 0, i16 3
ret i16 %cond
}
define dso_local i16 @testExtendSignBit(i16 %a) {
; CHECK-LABEL: testExtendSignBit:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: inv r12
; CHECK-NEXT: swpb r12
; CHECK-NEXT: mov.b r12, r12
; CHECK-NEXT: clrc
; CHECK-NEXT: rrc r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: ret
entry:
%cmp = icmp sgt i16 %a, -1
%cond = select i1 %cmp, i16 1, i16 0
ret i16 %cond
}
define dso_local i16 @testShiftAnd_0(i16 %a) {
; CHECK-LABEL: testShiftAnd_0:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: swpb r12
; CHECK-NEXT: sxt r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: ret
entry:
%cmp = icmp slt i16 %a, 0
%cond = select i1 %cmp, i16 -1, i16 0
ret i16 %cond
}
define dso_local i16 @testShiftAnd_1(i16 %a) {
; CHECK-LABEL: testShiftAnd_1:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: swpb r12
; CHECK-NEXT: mov.b r12, r12
; CHECK-NEXT: clrc
; CHECK-NEXT: rrc r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: ret
entry:
%cmp = icmp slt i16 %a, 0
%cond = select i1 %cmp, i16 1, i16 0
ret i16 %cond
}
define dso_local i16 @testShiftAnd_2(i16 %a) {
; CHECK-LABEL: testShiftAnd_2:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: swpb r12
; CHECK-NEXT: mov.b r12, r12
; CHECK-NEXT: clrc
; CHECK-NEXT: rrc r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: and #2, r12
; CHECK-NEXT: ret
entry:
%cmp = icmp slt i16 %a, 0
%cond = select i1 %cmp, i16 2, i16 0
ret i16 %cond
}
define dso_local i16 @testShiftAnd_3(i16 %a) {
; CHECK-LABEL: testShiftAnd_3:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: swpb r12
; CHECK-NEXT: sxt r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: rra r12
; CHECK-NEXT: and #3, r12
; CHECK-NEXT: ret
entry:
%cmp = icmp slt i16 %a, 0
%cond = select i1 %cmp, i16 3, i16 0
ret i16 %cond
}
define dso_local i16 @testShiftAnd_4(i16 %a, i16 %b) {
spatelUnsubmitted
Not Done
ReplyInline Actions

Is it intentional that all tests use the i16 type? Would it be beneficial/increase test coverage to include other common types like i32/i64?

spatel: Is it intentional that all tests use the i16 type? Would it be beneficial/increase test…
joanlluchAuthorUnsubmitted
Not Done
ReplyInline Actions

Yes, the i16 type is intentional. The target does not natively support larger types, so larger types are expanded into operations with register pairs requiring additional transformations, or are custom lowered, which confuse the purposes of this patch.

joanlluch: Yes, the i16 type is intentional. The target does not natively support larger types, so larger…
; CHECK-LABEL: testShiftAnd_4:
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: mov r12, r14
; CHECK-NEXT: mov #1, r12
; CHECK-NEXT: cmp r14, r13
; CHECK-NEXT: jl .LBB8_2
; CHECK-NEXT: ; %bb.1: ; %entry
; CHECK-NEXT: clr r12
; CHECK-NEXT: .LBB8_2: ; %entry
; CHECK-NEXT: add r12, r12
; CHECK-NEXT: add r12, r12
; CHECK-NEXT: add r12, r12
; CHECK-NEXT: add r12, r12
; CHECK-NEXT: add r12, r12
; CHECK-NEXT: ret
entry:
%cmp = icmp sgt i16 %a, %b
%cond = select i1 %cmp, i16 32, i16 0
ret i16 %cond
}