This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Add builtins for FP conversions
ClosedPublic

Authored by sdesmalen on Apr 15 2020, 1:40 PM.

Details

Summary

This adds the flag IsOverloadCvt which tells CGBulitin to use
the result type and the type of the last operand as the
overloaded types for the LLVM IR intrinsic.

This also adds the flag IsFPConvert, which is needed to avoid
converting the predicate of the operation from svbool_t to
a predicate with fewer lanes, as the LLVM IR intrinsics use
the <vscale x 16 x i1> as the predicate.

Diff Detail

Event Timeline

sdesmalen created this revision.Apr 15 2020, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 1:40 PM
Herald added a subscriber: tschuett. · View Herald Transcript

as the LLVM IR intrinsics use the <vscale x 16 x i1> as the predicate.

Why are the fp conversion intrinsics special here? Should we fix the LLVM intrinsic definitions?

as the LLVM IR intrinsics use the <vscale x 16 x i1> as the predicate.

Why are the fp conversion intrinsics special here? Should we fix the LLVM intrinsic definitions?

I questioned that as well, but didn't really want to complicate this patch and also wasn't sure whether it is worth it because the convert intrinsics are really only ever used by the ACLE.
On the other hand, it would let us get rid of the IsFPConvert flag and the definition would better match the behaviour of the instruction.

I'm happy to change their definitions and remove the IsFPConvert flag if that has your preference.

I think the predicate type is something we should fix. Even if it's not causing problems now, it seems like the sort of thing that will bite us later.

It doesn't necessarily need to block this patch, I guess, but it at least needs a good comment in EmitAArch64SVEBuiltinExpr explaining what's happening.

clang/utils/TableGen/SveEmitter.cpp
1045

Not worth implementing these separately? I guess I can see why it wouldn't be profitable.

sdesmalen marked an inline comment as done.Apr 17 2020, 2:18 PM

I think the predicate type is something we should fix. Even if it's not causing problems now, it seems like the sort of thing that will bite us later.

Yes I agree, I've implemented this in D78402

clang/utils/TableGen/SveEmitter.cpp
1045

We currently don't really optimise the _x (MergeAny) case yet for other intrinsics, so addressing this seems more trouble than it's worth, specifically because the merging with undef for only the top/odd lanes would not be trivial to represent.

sdesmalen updated this revision to Diff 258426.Apr 17 2020, 2:25 PM
  • Added comment to CGBuiltin explaining that the special case for IsFPConvert will be removed in a follow-up patch.
efriedma accepted this revision.Apr 17 2020, 2:48 PM

LGTM

clang/utils/TableGen/SveEmitter.cpp
1045

Okay.

This revision is now accepted and ready to land.Apr 17 2020, 2:48 PM
This revision was automatically updated to reflect the committed changes.