This is an archive of the discontinued LLVM Phabricator instance.

Differential D86489

[PowerPC] Add addtional test that retroactively catches PR47259
ClosedPublic

Authored by Bdragon28 on Aug 24 2020, 2:42 PM.

Details

Reviewers
hans
ZhangKang
Commits
rGf07b95e8bcd1: [PowerPC] Add addtional test that retroactively catches PR47259
Summary

Due to the unfortunate way the bug could only be triggered when reading SPRG[0-3] into a register lower than %r4 with the "mfsprg %rX, 0" syntax, the tests did not detect it.

(It could not be triggered for "mfsprg0, %r2" because that pattern was already in the table, so the earlier "correct" match took effect)

As a canary, add an intentionally ambiguous "mfsprg 2, 2" and "mtsprg 2, 2" check that would have caught the problem.

Diff Detail

Event Timeline

Bdragon28 created this revision.Aug 24 2020, 2:42 PM
Herald added subscribers: llvm-commits, shchenz, kbarton, nemanjai. · View Herald TranscriptAug 24 2020, 2:42 PM
Bdragon28 requested review of this revision.Aug 24 2020, 2:42 PM
Herald added a subscriber: • wuzish. · View Herald TranscriptAug 24 2020, 2:42 PM
Bdragon28 edited the summary of this revision. (Show Details)Aug 24 2020, 2:44 PM
Harbormaster completed remote builds in B69367: Diff 287491.Aug 24 2020, 3:27 PM
ZhangKang added inline comments.Aug 24 2020, 6:31 PM
llvm/test/MC/PowerPC/ppc64-encoding-ext.s
3611

I think it's better to use mtsprg 2, %r2。

Bdragon28 added inline comments.Aug 25 2020, 12:09 PM
llvm/test/MC/PowerPC/ppc64-encoding-ext.s
3611

*normally* I would agree here, but the whole point of the test is to catch a screwed up interpretation of instruction alias matching, so making it as vauge as possible is part of the point of the test.

ZhangKang accepted this revision.Aug 25 2020, 10:26 PM

LGTM. Thank for adding the test.

This revision is now accepted and ready to land.Aug 25 2020, 10:26 PM
Closed by commit rGf07b95e8bcd1: [PowerPC] Add addtional test that retroactively catches PR47259 (authored by Bdragon28). · Explain WhyDec 30 2020, 1:24 PM
This revision was automatically updated to reflect the committed changes.
Bdragon28 added a commit: rGf07b95e8bcd1: [PowerPC] Add addtional test that retroactively catches PR47259.

Revision Contents

PathSize
llvm/
test/
MC/
PowerPC/
10 lines

Diff 314146

llvm/test/MC/PowerPC/ppc64-encoding-ext.s

Show First 20 Lines • Show All 3,565 Lines • ▼ Show 20 Lines
# CHECK-BE: mfspr 4, 274 # encoding: [0x7c,0x92,0x42,0xa6] # CHECK-BE: mfspr 4, 274 # encoding: [0x7c,0x92,0x42,0xa6]
# CHECK-LE: mfspr 4, 274 # encoding: [0xa6,0x42,0x92,0x7c] # CHECK-LE: mfspr 4, 274 # encoding: [0xa6,0x42,0x92,0x7c]
mfsprg %r4, 2 mfsprg %r4, 2
# CHECK-BE: mfspr 4, 275 # encoding: [0x7c,0x93,0x42,0xa6] # CHECK-BE: mfspr 4, 275 # encoding: [0x7c,0x93,0x42,0xa6]
# CHECK-LE: mfspr 4, 275 # encoding: [0xa6,0x42,0x93,0x7c] # CHECK-LE: mfspr 4, 275 # encoding: [0xa6,0x42,0x93,0x7c]
mfsprg %r4, 3 mfsprg %r4, 3
# Bug PR47259
# CHECK-BE: mfspr 2, 274 # encoding: [0x7c,0x52,0x42,0xa6]
# CHECK-LE: mfspr 2, 274 # encoding: [0xa6,0x42,0x52,0x7c]
mfsprg 2, 2
# CHECK-BE: mfspr 2, 272 # encoding: [0x7c,0x50,0x42,0xa6] # CHECK-BE: mfspr 2, 272 # encoding: [0x7c,0x50,0x42,0xa6]
# CHECK-LE: mfspr 2, 272 # encoding: [0xa6,0x42,0x50,0x7c] # CHECK-LE: mfspr 2, 272 # encoding: [0xa6,0x42,0x50,0x7c]
mfsprg0 %r2 mfsprg0 %r2
# CHECK-BE: mfspr 2, 273 # encoding: [0x7c,0x51,0x42,0xa6] # CHECK-BE: mfspr 2, 273 # encoding: [0x7c,0x51,0x42,0xa6]
# CHECK-LE: mfspr 2, 273 # encoding: [0xa6,0x42,0x51,0x7c] # CHECK-LE: mfspr 2, 273 # encoding: [0xa6,0x42,0x51,0x7c]
mfsprg1 %r2 mfsprg1 %r2
# CHECK-BE: mfspr 2, 274 # encoding: [0x7c,0x52,0x42,0xa6] # CHECK-BE: mfspr 2, 274 # encoding: [0x7c,0x52,0x42,0xa6]
# CHECK-LE: mfspr 2, 274 # encoding: [0xa6,0x42,0x52,0x7c] # CHECK-LE: mfspr 2, 274 # encoding: [0xa6,0x42,0x52,0x7c]
Show All 13 Lines
# CHECK-BE: mtspr 274, 4 # encoding: [0x7c,0x92,0x43,0xa6] # CHECK-BE: mtspr 274, 4 # encoding: [0x7c,0x92,0x43,0xa6]
# CHECK-LE: mtspr 274, 4 # encoding: [0xa6,0x43,0x92,0x7c] # CHECK-LE: mtspr 274, 4 # encoding: [0xa6,0x43,0x92,0x7c]
mtsprg 2, %r4 mtsprg 2, %r4
# CHECK-BE: mtspr 275, 4 # encoding: [0x7c,0x93,0x43,0xa6] # CHECK-BE: mtspr 275, 4 # encoding: [0x7c,0x93,0x43,0xa6]
# CHECK-LE: mtspr 275, 4 # encoding: [0xa6,0x43,0x93,0x7c] # CHECK-LE: mtspr 275, 4 # encoding: [0xa6,0x43,0x93,0x7c]
mtsprg 3, %r4 mtsprg 3, %r4
# Bug PR47259
# CHECK-BE: mtspr 274, 2 # encoding: [0x7c,0x52,0x43,0xa6]
# CHECK-LE: mtspr 274, 2 # encoding: [0xa6,0x43,0x52,0x7c]
mtsprg 2, 2
ZhangKangUnsubmitted
Not Done
ReplyInline Actions

I think it's better to use mtsprg 2, %r2。

ZhangKang: I think it's better to use `mtsprg 2, %r2`。
Bdragon28AuthorUnsubmitted
Done
ReplyInline Actions

*normally* I would agree here, but the whole point of the test is to catch a screwed up interpretation of instruction alias matching, so making it as vauge as possible is part of the point of the test.

Bdragon28: *normally* I would agree here, but the whole point of the test is to catch a screwed up…
# CHECK-BE: mtspr 272, 4 # encoding: [0x7c,0x90,0x43,0xa6] # CHECK-BE: mtspr 272, 4 # encoding: [0x7c,0x90,0x43,0xa6]
# CHECK-LE: mtspr 272, 4 # encoding: [0xa6,0x43,0x90,0x7c] # CHECK-LE: mtspr 272, 4 # encoding: [0xa6,0x43,0x90,0x7c]
mtsprg0 %r4 mtsprg0 %r4
# CHECK-BE: mtspr 273, 4 # encoding: [0x7c,0x91,0x43,0xa6] # CHECK-BE: mtspr 273, 4 # encoding: [0x7c,0x91,0x43,0xa6]
# CHECK-LE: mtspr 273, 4 # encoding: [0xa6,0x43,0x91,0x7c] # CHECK-LE: mtspr 273, 4 # encoding: [0xa6,0x43,0x91,0x7c]
mtsprg1 %r4 mtsprg1 %r4
▲ Show 20 Lines • Show All 95 Lines • Show Last 20 Lines