This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Upgrade VectorType tests to test new types
ClosedPublic

Authored by ctetreau on Apr 24 2020, 2:40 PM.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 24 2020, 2:40 PM
sdesmalen added inline comments.Apr 27 2020, 1:51 PM
llvm/unittests/IR/VectorTypesTest.cpp
31

I think you can just write *RHS (operator<< is overloaded for const Type &T)

152

Do you not want to test for getElementCount here?

ctetreau marked an inline comment as done.Apr 27 2020, 3:24 PM
ctetreau added inline comments.
llvm/unittests/IR/VectorTypesTest.cpp
152

I chose not to because both types are instances of ScalableVectorType, and by definition that means that EC.Scalable is true. But then again, I suppose the purpose of tests is to ensure that stays true. I'll make the change.

ctetreau updated this revision to Diff 260489.Apr 27 2020, 3:55 PM

address code review issues

ctetreau updated this revision to Diff 260666.Apr 28 2020, 9:30 AM

address remaining code review issues

sdesmalen accepted this revision.Apr 28 2020, 1:28 PM

LGTM!

llvm/unittests/IR/VectorTypesTest.cpp
152

Right, I misread this as getNumElements. In that case both are indeed fine though.

This revision is now accepted and ready to land.Apr 28 2020, 1:28 PM
ctetreau marked 2 inline comments as done.Apr 28 2020, 2:20 PM
ctetreau added inline comments.
llvm/unittests/IR/VectorTypesTest.cpp
152

In my mind, the most important thing is that is has a different name to prevent copy/paste and template duck typing issues. however, if after having seen it in the wild, you think it's too similar to getNumElements(), I'm open to changing it.

This revision was automatically updated to reflect the committed changes.
ctetreau marked an inline comment as done.
fhahn added a subscriber: fhahn.Jun 2 2020, 3:02 AM
fhahn added inline comments.
llvm/unittests/IR/VectorTypesTest.cpp
231

This seems to cause the following warning on GreenDragon (
http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15985/console)

It would be great if you could take a look.

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-expensive/llvm-project/llvm/unittests/IR/VectorTypesTest.cpp:231:39: warning: suggest braces around initialization of subobject [-Wmissing-braces]
  std::array<VectorType *, 8> VTys = {VectorType::get(Int16Ty, {4, true}),
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                      {
efriedma added inline comments.Jun 2 2020, 1:07 PM
llvm/unittests/IR/VectorTypesTest.cpp
231

Those warnings pop up all over the place (including other places in that build) if you use clang 5.0 or older to build clang. clang prints a warning because std::array is actually a struct that contains a C array. Newer versions suppress the warning, though.

Maybe we should teach CMake to pass -Wno-missing-braces to older versions of clang.