This is an archive of the discontinued LLVM Phabricator instance.

[SVE][AArch64] Adding patterns for while intrinsics
ClosedPublic

Authored by mgudim on Oct 16 2019, 11:03 AM.

Details

Summary

This is a follow-up to https://reviews.llvm.org/D68476.

Straight-forward pattern matching for while intrinsics.

Diff Detail

Event Timeline

mgudim created this revision.Oct 16 2019, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 11:03 AM
This comment was removed by mgudim.
amehsan accepted this revision.Oct 30 2019, 7:50 PM
amehsan added a subscriber: sdesmalen.

LGTM. @sdesmalen @huntergr please let us know if you have any concern

This revision is now accepted and ready to land.Oct 30 2019, 7:50 PM
sdesmalen added inline comments.Oct 31 2019, 1:33 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1501

These instructions should not be removed. (This is also guarded by the assembler/disassembler tests)

llvm/lib/Target/AArch64/SVEInstrFormats.td
3588

nit: Given that we chose to be explicit about adding the patterns (see https://reviews.llvm.org/D69128#1725072), it would be nice to do the same for these patterns.

mgudim updated this revision to Diff 227767.Nov 4 2019, 1:10 PM

Any thoughts about exposing the NZCV flags?

mgudim marked 2 inline comments as done.Nov 14 2019, 2:38 PM

Any thoughts about exposing the NZCV flags?

efriedma, sorry for the late reply. I see in .td files that the intrinsics define the NSCV flags, but I am not sure what you mean by "exposing" them. Could you please clarify?

If you look at SVE loop examples from ARM, they generally have a branch using the flags set by something like a whilelt; we should allow the user to replicate that somehow. I was thinking you could make them part of the return value, somehow, so the intrinsic returns two values. Maybe the whole value, or maybe just a boolean for some common use we expect; haven't really thought that part through.

Maybe that's not a good idea, though? I guess we could let the user write a regular cmp, and have a pass detect that it's redundant, somehow. Or maybe there's some other similar approach I haven't considered.

Hmm, I just dug up some examples using ACLE. It looks like the suggestion there is to use ptest on the result. The ptest can then be optimized away by the backend in common cases. I guess that works.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1504

Why are you using null_frag here?

Also, there aren't any tests for whilege/whilegt/whilehs/whilehi.

mgudim updated this revision to Diff 230156.Nov 19 2019, 3:16 PM

Put null_frag in SVE2 while intrinsics.

mgudim marked an inline comment as done.Nov 19 2019, 3:23 PM
mgudim added inline comments.
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1504

I did not want to touch SVE2 intrinsics, I changed these lines accidentally, sorry about that. The null_frag is necessary for .td files to compile since I changed the definitions of the multiclasses.

efriedma accepted this revision.Nov 19 2019, 3:32 PM

LGTM

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1504

Oh, I missed that the ge/gt/hs/hi variants are only part of SVE2. I guess that's okay for now, then.

@mgudim I've created a patch (D70909) also implementing these intrinsics but I was unaware of this patch, sorry about that. Can this be merged now? I can update D70909 to only add the SVE2 intrinsics .

@mgudim I've created a patch (D70909) also implementing these intrinsics but I was unaware of this patch, sorry about that. Can this be merged now? I can update D70909 to only add the SVE2 intrinsics .

@c-rhodes I will merge this patch tomorrow. Sorry for my long delay