Page MenuHomePhabricator

[VectorUtils] API for VFShape, update VFInfo.
ClosedPublic

Authored by fpetrogalli on Nov 20 2019, 2:34 PM.

Details

Summary
Summary:
This patch introduces an API to build and modify vector shapes.

The validity of a VFShape can be checked with the
`hasValidParameterList` method, which is also run in an assertion each
time a VFShape is modified.

The field VFISAKind has been moved to VFInfo under the assumption that
different ISAs can map to the same VFShape (as it can be in the case
of vector extensions with the same registers size, for example AVX and
AVX2).

Reviewers: sdesmalen, jdoerfert, simoll, hsaito

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D70513

Diff Detail

Event Timeline

fpetrogalli created this revision.Nov 20 2019, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 2:34 PM
fpetrogalli marked an inline comment as done.Nov 20 2019, 2:39 PM
fpetrogalli added inline comments.
llvm/unittests/Analysis/VectorUtilsTest.cpp
315

This should be /*Parameters*/, not /*HasGlobalPredicate*/. Same for all uses below.

fpetrogalli marked an inline comment as not done.Nov 20 2019, 2:39 PM
fpetrogalli marked an inline comment as done.

Fully implement and test the API.

This patch is ready to be reviewed.

Thanks,

Francesco

In this patch I have achieved better unit tests granularity by moving
the EXPECT_* macros from the test fixture class into the actual
test, so that in case of failure the log message points at the right
line in the code (otherwise it always points at the EXPECT_* in the
method used to run the check in the class definition of the test
fixture).

fpetrogalli retitled this revision from [VectorUtils] API for VFShape. to [VectorUtils] API for VFShape, update VFInfo..Nov 20 2019, 8:08 PM
fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli marked an inline comment as not done.
fpetrogalli marked an inline comment as done.

In this last update I have:

  1. Removed an assertion in updateParameter that was checking a condition for VFParamKind::GlobalPredicate already checked in hasValidParameterList.
  1. Renamed a method in the test for better readability of the code (from isValidParameterList to validParams).

Thanks,

Francesco

In this update I have removed empty statements in the tests. (facepalm)

Thanks,

Francesco

Update (and rename) the get method to use ElementCount instead of VF
and IsScalable separately.

fpetrogalli marked an inline comment as done.Nov 22 2019, 9:50 AM
fpetrogalli added inline comments.
llvm/include/llvm/Analysis/VectorUtils.h
83–85

Notice that I am not using ElementCount here to realize VF and IsScalable because it would delete the default constructors associated to VFShape and VFInfo.

Looks fine from my perspective. Anyone else has input on this?

Allow GlobalPredicate parameters in any position of the signature.

sdesmalen added inline comments.Dec 4 2019, 8:59 AM
llvm/include/llvm/Analysis/VectorUtils.h
91

nit: can you add a newline between the previous and next method?

llvm/lib/Analysis/VectorUtils.cpp
1189

should this be an assert?

1191

nit: can you add a newline above?

1192

nit: Nothign -> Nothing

1222

Can you add a function here that calculates this, per supported VFABI?

llvm/unittests/Analysis/VectorUtilsTest.cpp
311

nit: this could do with some newlines.

fpetrogalli marked 8 inline comments as done.

Address review comments from @sdesmalen.

llvm/lib/Analysis/VectorUtils.cpp
1189

I have updated also the tests replacing EXPECT_FALSE with EXPECT_DEATH, to make sure the assert is not removed.

1222

Marking this as done as the most recent version of the patch doesn't have any concept for VFABI for GlobalPredicate, as the global predicate is allowed to be anywhere in the signature, as long as it is unique.

sdesmalen accepted this revision.Dec 4 2019, 11:53 AM

Thanks for the fixes @fpetrogalli, LGTM!

This revision is now accepted and ready to land.Dec 4 2019, 11:53 AM
This revision was automatically updated to reflect the committed changes.