This is an archive of the discontinued LLVM Phabricator instance.

[ARM/AArch64] - Remove some undefined behaviour when lowering vector shifts
ClosedPublic

Authored by LukeCheeseman on Jul 22 2015, 3:41 AM.

Details

Summary

When lowering vector shifts a check is performed to see if the value to shift by is an immediate, in this check the value is negated and stored in and int64_t. The value can be -2^63 yet the result cannot be stored in an int64_t and this gives some undefined behaviour. The regression test included exposes this behaviour, this test fails in the current release build with assertions when built using clang as the check for an immediate passes when it should not due to this undefined behaviour. The negation is only necessary when the values is within a certain range and so it should not need to negate -2^63, this patch introduces this and also a regression test.

Diff Detail

Event Timeline

LukeCheeseman retitled this revision from to [ARM/AArch64] - Remove some undefined behaviour when lowering vector shifts.
LukeCheeseman updated this object.
LukeCheeseman added a subscriber: llvm-commits.

No AArch64 tests?

test/CodeGen/ARM/neon_vshl_minint.ll
2

You're not using prefixes ACORE/MCORE?

Removed some unnecessary check prefixes in tests.

Hi Renato

Thanks for pointing out the unnecessary check prefixes. I haven't added a testcase for the AArch64 backend as it isn't possible to hit the same undefined behaviour as in the ARM backend. The negation is only done in isVShiftRImm when isIntrinsic is true and the only call site in the AArch64 backend passes this as false so the negation isn't performed.

Thanks,
Luke

Thanks for pointing out the unnecessary check prefixes. I haven't added a testcase for the AArch64 backend as it isn't possible to hit the same undefined behaviour as in the ARM backend. The negation is only done in isVShiftRImm when isIntrinsic is true and the only call site in the AArch64 backend passes this as false so the negation isn't performed.

So, why not an an assert for now, and point to the same implementation in ARM mode for when it's needed?

--renato

Do you mean assert that isIntrinsic is always false?

Thanks,
Luke

Do you mean assert that isIntrinsic is always false?

If it's unlikely to ever be used, then yes.

Removed isIntrinsic boolean from isVShift{R, L}Imm in AArch64 backend

rengolin accepted this revision.Jul 23 2015, 9:57 AM
rengolin added a reviewer: rengolin.

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 23 2015, 9:57 AM
This revision was automatically updated to reflect the committed changes.