[PATCH] D98856: Always emit error for wrong interfaces to scalable vectors, unless cmdline flag is passed.

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:31-35
+namespace TypeSizeClOpt {
+/// The ScalableErrorAsWarning is a temporary measure to suppress errors from
+/// using the wrong interface.
+extern cl::opt<bool> ScalableErrorAsWarning;
+} // namespace TypeSizeClOpt
----------------
paulwalker-arm wrote:
> Why is this need here? Can it just be externs where it's accessed?
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.


================
Comment at: llvm/lib/CodeGen/ValueTypes.cpp:28
+  if (isScalableVector()) {
+    if (llvm::TypeSizeClOpt::ScalableErrorAsWarning)
+      WithColor::warning()
----------------
paulwalker-arm wrote:
> 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()
> }
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98856/new/

https://reviews.llvm.org/D98856