Page MenuHomePhabricator

[AArch64] Fix fast-isel of cbz of i1, i8, i16
ClosedPublic

Authored by olista01 on Oct 23 2014, 7:04 AM.

Details

Summary

This fixes a miscompilation in the AArch64 fast-isel which was triggered when a branch is based on an icmp with condition eq or ne, and type i1, i8 or i16. The cbz instruction compares the whole 32-bit register, so values with the bottom 1, 8 or 16 bits clear would cause the wrong branch to be taken.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 15325.Oct 23 2014, 7:04 AM
olista01 retitled this revision from to [AArch64] Fix fast-isel of cbz of i1, i8, i16.
olista01 updated this object.
olista01 edited the test plan for this revision. (Show Details)
olista01 set the repository for this revision to rL LLVM.
olista01 added a subscriber: Unknown Object (MLST).

Hi Oliver,

This looks fine, just one very minor quibble.

lib/Target/AArch64/AArch64FastISel.cpp
2184

BW < 32?

t.p.northover accepted this revision.Oct 23 2014, 9:46 AM
t.p.northover added a reviewer: t.p.northover.

Go ahead and commit if you want (with or without the suggestion, realistically).

This revision is now accepted and ready to land.Oct 23 2014, 9:46 AM
ributzka accepted this revision.Oct 23 2014, 10:52 AM
ributzka edited edge metadata.

Hi Oliver,

thanks for catching this. I only have a minor comment inline.
It would be great if you also could optimize the i1 case by emitting a 'tbs' instead. Maybe in a separate commit?

Thanks and LGTM.

-Juergen

lib/Target/AArch64/AArch64FastISel.cpp
2185

getValueType doesn't use kill flags - a simple 'true' will do here.

test/CodeGen/AArch64/fast-isel-cbz.ll
3

This case could be optimized to use tbz instead.

olista01 closed this revision.Oct 24 2014, 3:05 AM

Thanks, committed revision 220553 with both of your suggested changes.