Page MenuHomePhabricator

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

Authored by efriedma on Fri, Jan 11, 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.Fri, Jan 11, 12:03 PM

rebase now that D56544 has landed?

SjoerdMeijer added inline comments.Mon, Jan 14, 7:17 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
2721 ↗(On Diff #181348)

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 ↗(On Diff #181348)

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.Mon, Jan 14, 12:52 PM
efriedma added inline comments.
lib/Target/AArch64/AArch64ISelLowering.cpp
2726 ↗(On Diff #181348)

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.Mon, Jan 14, 1:13 PM
This revision was automatically updated to reflect the committed changes.