This is an archive of the discontinued LLVM Phabricator instance.

[VP] Add more cast VPintrinsic and docs.
ClosedPublic

Authored by ym1813382441 on Mar 22 2022, 11:21 PM.

Details

Summary

Add vp.fptoui, vp.uitofp, vp.fptrunc, vp.fpext, vp.trunc, vp.zext, vp.sext, vp.ptrtoint, vp.inttoptr intrinsic and docs.

Diff Detail

Event Timeline

ym1813382441 created this revision.Mar 22 2022, 11:21 PM
ym1813382441 requested review of this revision.Mar 22 2022, 11:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 11:21 PM
ym1813382441 added a project: Restricted Project.Mar 23 2022, 1:29 AM

Verifier.cpp needs to be updated to verify the element counts match between the source and destination and that they have integer/fp types where they should. fpext, fptrunc, trunc, zext, and sext also need to verify that the element size is increasing/decreasing like its supposed to.

llvm/docs/LangRef.rst
20711

"type then" -> "type, then"

20712

Here too

20767

"pointer then" -> "pointer, then"

20768

Here too

llvm/include/llvm/IR/VPIntrinsics.def
332

Usually you shouldn't make formatting changes to code you aren't touching. If you want to extract this from this patch and just commit it, that would be fine.

356

Same here

Verifier.cpp needs to be updated to verify the element counts match between the source and destination and that they have integer/fp types where they should. fpext, fptrunc, trunc, zext, and sext also need to verify that the element size is increasing/decreasing like its supposed to.

Element count might be checked already by Verifier::visitVPIntrinsic.

Update docs and format.

Update Verifier.cpp to check VPCastIntrinsic.

ym1813382441 marked 6 inline comments as done.Mar 29 2022, 8:15 PM
This revision is now accepted and ready to land.Mar 30 2022, 5:25 PM

LGTM

I do not appear to have commit access. I am applying recently. Could you land this for me?
For Author: yanming <ming.yan@terapines.com>

frasercrmck requested changes to this revision.Mar 31 2022, 12:55 AM
frasercrmck added inline comments.
llvm/docs/LangRef.rst
20258

must be vectors

20259

in trunc we explicitly say it cannot be a no-op cast. Should we say that here too?

20282

these select ops should be <4 x i32>

20310

vectors again.

20373

trunc -> fptrunc

20404

v16f64.v16f32?

20405

other way round - nxv4f64.nxv4f32?

20502

select operands should be <4 x i32> (I just pushed cc67a8fcf148 to fix it in vp.fptosi.)

llvm/include/llvm/IR/VPIntrinsics.def
307

Can't we have HELPER_REGISTER_INT_CAST_VP or something? This just makes it harder to navigate the codebase imo.

llvm/lib/IR/Verifier.cpp
5604

I'd like to see some tests in test/Verifier/invalid-vp-intrinsics.ll for this

5607

You don't need {} in any of these switch cases

llvm/test/Verifier/vp-intrinsics.ll
60–61

Feels to me like the name test_vp_int_fp_conversions implies it's testing only int<->fp conversions. Maybe we should have test_vp_int_conversions for sext/zext/trunc/ptrtoint/inttoptr and test_vp_fp_conversions for fpext/fptrunc?

This revision now requires changes to proceed.Mar 31 2022, 12:55 AM
ym1813382441 added inline comments.Mar 31 2022, 3:04 AM
llvm/docs/LangRef.rst
20259

vp.trunc must not be a no-op cast, but this is vp.zext, its behavior should be consistent with the basic zext instruction. The basic zext instruction document does not indicate whether it is no-op cast. My understanding is that different targets have different operations.

20282

sorry, this is my carelessness.

llvm/include/llvm/IR/VPIntrinsics.def
307

I will separate this macro, thanks.

llvm/lib/IR/Verifier.cpp
5607

This is my personal code style, I will remove it.

llvm/test/Verifier/vp-intrinsics.ll
60–61

I want to use test_vp_conversions to include all, is OK?

Fix docs and code style.

ym1813382441 marked 10 inline comments as done.Mar 31 2022, 3:09 AM
frasercrmck accepted this revision.Mar 31 2022, 3:54 AM

LGTM, thanks!

llvm/docs/LangRef.rst
20259

Ah right I see I misunderstood "no-op cast" then - I thought it was another way of saying the type sizes must be equivalent. Thanks!

20282

easily done :)

llvm/test/Verifier/vp-intrinsics.ll
60–61

Fine by me, I don't think it's very important in this test.

This revision is now accepted and ready to land.Mar 31 2022, 3:54 AM
This revision was landed with ongoing or failed builds.Mar 31 2022, 6:17 PM
This revision was automatically updated to reflect the committed changes.