This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Explicitly use v1i64 type for llvm.aarch64.neon.abs.i64 .
ClosedPublic

Authored by efriedma on Jan 11 2019, 12:03 PM.

Details

Summary

Otherwise, with D56544, the intrinsic will be expanded to an integer csel, which is probably not what the user expected. This matches the general convention of using "v1" types to represent scalar integer operations in vector registers.

While I'm here, also add some error checking so we don't generate illegal ABS nodes.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Jan 11 2019, 12:03 PM

rebase now that D56544 has landed?

SjoerdMeijer added inline comments.Jan 14 2019, 7:17 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
2721

If I understand things correctly, D56544 is committed, so that means this intrinsic is lowered to different ISD nodes but codegen is the same. And a quick grep suggests this is already covered by test arm64-vabs.ll.

2726

And this is covered by arm64-vabs.ll too. I.e. the tests with llvm.aarch64.neon.abs.v4i32 etc.

In that case my only nitpick is that we don't need all the curly brackets here but it LGTM to me otherwise.

efriedma marked an inline comment as done.Jan 14 2019, 12:52 PM
efriedma added inline comments.
lib/Target/AArch64/AArch64ISelLowering.cpp
2726

Normally I don't like mixing brace styles in a single-if-else (it makes it more confusing to figure out where the chain ends). Not sure how else I would refactor to reduce the number of braces.

This revision is now accepted and ready to land.Jan 14 2019, 1:13 PM
This revision was automatically updated to reflect the committed changes.