Page MenuHomePhabricator

Always emit error for wrong interfaces to scalable vectors, unless cmdline flag is passed.
ClosedPublic

Authored by sdesmalen on Mar 18 2021, 5:11 AM.

Details

Summary

In order to bring up scalable vector support in LLVM incrementally,
we introduced behaviour to emit a warning, instead of an error, when
asking the wrong question of a scalable vector, like asking for the
fixed number of elements.

This patch puts that behaviour under a flag. The default behaviour is
that the compiler will always error, which means that all LLVM unit
tests and regression tests will now fail when a code-path is taken that
still uses the wrong interface.

The behaviour to demote an error to a warning can be individually enabled
for tools that want to support experimental use of scalable vectors.
This patch enables that behaviour when driving compilation from Clang.
This means that for users who want to try out scalable-vector support,
fixed-width codegen support, or build user-code with scalable vector
intrinsics, Clang will not crash and burn when the compiler encounters
such a case.

This allows us to do away with the following pattern in many of the SVE tests:

RUN: .... 2>%t
RUN: cat %t | FileCheck --check-prefix=WARN
WARN-NOT: warning: ...

The behaviour to emit warnings is only temporary and we expect this flag
to be removed in the future when scalable vector support is more stable.

This patch also has fixes the following tests:
unittests:

ScalableVectorMVTsTest.SizeQueries
SelectionDAGAddressAnalysisTest.unknownSizeFrameObjects
AArch64SelectionDAGTest.computeKnownBitsSVE_ZERO_EXTEND_VECTOR_INREG

regression tests:

Transforms/InstCombine/vscale_gep.ll

Diff Detail

Event Timeline

sdesmalen created this revision.Mar 18 2021, 5:11 AM
sdesmalen requested review of this revision.Mar 18 2021, 5:11 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 18 2021, 5:11 AM
sdesmalen updated this revision to Diff 331820.Mar 19 2021, 4:23 AM

Rebased after D98736 already fixed one of the tests.

ctetreau accepted this revision.Mar 19 2021, 9:40 AM

lgtm

llvm/lib/CodeGen/ValueTypes.cpp
17 ↗(On Diff #331820)

Out of curiosity, what is the eventual plan for this function? Does it go away, or will it just assert if this is a scalable vector?

This revision is now accepted and ready to land.Mar 19 2021, 9:40 AM
paulwalker-arm added inline comments.Mar 19 2021, 9:41 AM
clang/lib/Driver/ToolChains/Clang.cpp
5064–5067

Are there any concerns related to LTO here? Could we live with LTO triggering errors for the invalid uses? Part of me thinks this is reasonable given the clang exception is more about ensuring we can continue active development until we're ready to press the "it's supported" switch.

llvm/include/llvm/Support/TypeSize.h
30–34

Why is this need here? Can it just be externs where it's accessed?

llvm/lib/CodeGen/ValueTypes.cpp
28 ↗(On Diff #331820)

I guess related to my comment for TypeSize.h but I'm wondering if it's better to move all error reporting into TypeSize.cpp. For example:

if (isScalableVector())
  reportInvalidSizeRequest("EVT::getVectorNumElements()", "EVT::getVectorElementCount()")

reportInvalidSizeRequest(string bad_func, string good_fun) {
#ifndef STRICT_FIXED_SIZE_VECTORS
  if (ScalableErrorAsWarning)
    warning(don't use badfunc, use good_fun instead)
  else
#endif
    Error()
}
ctetreau added inline comments.Mon, Mar 22, 9:15 AM
llvm/include/llvm/Support/TypeSize.h
30–34

It could, but that would be annoying. It would be nice if you could just include the header and have this symbol in scope.

Whether or not it should be convenient to use this option is a valid question though. I'm fine either way.

llvm/lib/CodeGen/ValueTypes.cpp
28 ↗(On Diff #331820)

I guess if we did this, then the definitions for EVT::getVectorNumElements and the TypeSize cast to unsigned to stay in the header. Then, if STRICT_FIXED_SIZE_VECTORS is defined, it would be easy to inline the function body as it's all in the header, resulting in a real "pro" to defining STRICT_FIXED_SIZE_VECTORS and having your code work with it. I'm fine either way.

It's only two functions, and unlikely to increase to more, so you could make the case that it's not worth bothering over.

sdesmalen updated this revision to Diff 333010.Wed, Mar 24, 8:49 AM

Moved error reporting to llvm::reportInvalidSizeRequest in TypeSize.cpp.

sdesmalen added inline comments.Wed, Mar 24, 9:01 AM
clang/lib/Driver/ToolChains/Clang.cpp
5064–5067

Are there any concerns related to LTO here?

Yes, probably.

Could we live with LTO triggering errors for the invalid uses? Part of me thinks this is reasonable given the clang exception is more about ensuring we can continue active development until we're ready to press the "it's supported" switch.

Yes, that's my thinking about this as well. When the compiler is ready to be used in production, then an actual runtime compiler error (the default) would be appropriate which would lead to a bug-report that'd we need to go off and fix. During development of scalable vector support, we just want to have some extra flexibility, because we know not all code-paths are covered. So while we use it for those purposes, the recommendation is not to use LTO.

sdesmalen updated this revision to Diff 333018.Wed, Mar 24, 9:11 AM
sdesmalen marked 5 inline comments as done.

Moved implementation of EVT::getVectorNumElements back to header.

sdesmalen added inline comments.Wed, Mar 24, 9:17 AM
llvm/lib/CodeGen/ValueTypes.cpp
17 ↗(On Diff #331820)

The eventual plan is for this function to go away, so we just have a single getElementCount interface, although this hasn't really been discussed yet.
We could also end up renaming it to getFixedVectorNumElements for convenience, at which point it always asserts if it is scalable.

paulwalker-arm accepted this revision.Thu, Mar 25, 5:30 AM
paulwalker-arm added inline comments.
llvm/include/llvm/Support/TypeSize.h
30

an

ctetreau accepted this revision.Tue, Mar 30, 4:05 PM
ctetreau added inline comments.
llvm/lib/CodeGen/ValueTypes.cpp
17 ↗(On Diff #331820)

ok, thanks.

This revision was landed with ongoing or failed builds.Fri, Apr 2, 3:23 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked an inline comment as done.Fri, Apr 2, 3:23 AM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5066

Does this introduce an option in the CC1 command line for most cases? We should shorten the CC1 command line for common cases...

sdesmalen added inline comments.Wed, Apr 7, 6:46 AM
clang/lib/Driver/ToolChains/Clang.cpp
5066

The option is purely temporary and will be removed in the future. I guess it depends on the meaning of the word 'common' in this context, but it enables experimental use of Clang for scalable vectors, which is common if the targets support them. We'd like to remove this flag sooner rather than later, although there is work involved in LLVM before we can remove it.

Are you specifically concerned with the length (as in: number of characters) of the cc1 command?

MaskRay added inline comments.Wed, Apr 7, 10:34 AM
clang/lib/Driver/ToolChains/Clang.cpp
5066

Are you specifically concerned with the length (as in: number of characters) of the cc1 command?

Yes.

I looked at a random SVE unrelated clang -### and found this option... Now I tried again and can't find the option now. Reducing CC1 length without making internal implementation difficult is generally good.