Page MenuHomePhabricator

[IR] Match intrinsic paramater by scalar/vectorwidth
ClosedPublic

Authored by RKSimon on Jan 23 2019, 3:25 AM.

Details

Summary

This patch replaces the existing LLVMVectorSameWidth matcher with LLVMScalarOrSameVectorWidth.

The matching args must be either scalars or vectors with the same number of elements, but in either case the scalar/element type can differ, specified by LLVMScalarOrSameVectorWidth.

I've updated the _overflow intrinsics to demonstrate this - allowing it to return a i1 or <N x i1> overflow result, matching the scalar/vectorwidth of the other (add/sub/mul) result type.

The masked load/store/gather/scatter intrinsics have also been updated to use this, although as we specify the reference type to be llvm_anyvector_ty we guarantee the mask will be <N x i1> so no change in behaviour

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jan 23 2019, 3:25 AM
andreadb accepted this revision.Jan 23 2019, 4:45 AM

The patch looks good to me. However, I don't claim to be an expert of this area. So getting another pair of eyes on this could be useful.

I was about to suggest to add a small example to the code comment of LLVMScalarOrSameVectorWidth. However, in retrospect, I think your comment is fine.

This revision is now accepted and ready to land.Jan 23 2019, 4:45 AM

This implies that we should change the LangRef for overflowing ops to match? Currently, there's no mention of vector types:
http://llvm.org/docs/LangRef.html#arithmetic-with-overflow-intrinsics

This implies that we should change the LangRef for overflowing ops to match? Currently, there's no mention of vector types:
http://llvm.org/docs/LangRef.html#arithmetic-with-overflow-intrinsics

If its alright, I'd prefer to wait on updating the docs until I have full testing of vector types in place, which would be in a followup patch.

spatel accepted this revision.Jan 23 2019, 7:22 AM

This implies that we should change the LangRef for overflowing ops to match? Currently, there's no mention of vector types:
http://llvm.org/docs/LangRef.html#arithmetic-with-overflow-intrinsics

If its alright, I'd prefer to wait on updating the docs until I have full testing of vector types in place, which would be in a followup patch.

Yes, that sounds ok to me. LGTM - see inline for a couple of nits.

include/llvm/IR/Intrinsics.td
163 ↗(On Diff #183073)

Not sure if I've ever looked at this code before, so it wasn't immediately clear what 'num' was referring to. Would it make sense to name that "index" or "opIndex"? (Similarly in the existing code around this).

lib/IR/Function.cpp
951 ↗(On Diff #183073)

use 'auto' for consistency with the other dyn_casts around here.

This revision was automatically updated to reflect the committed changes.