This is an archive of the discontinued LLVM Phabricator instance.

[llvm][VectorUtils] Tweak VFShape for scalable vector functions.
ClosedPublic

Authored by fpetrogalli on Jan 23 2020, 12:25 PM.

Details

Summary

This patch makes sure that the field VFShape.VF is greater than zero
when demangling the vector function name of scalable vector functions
encoded in the "vector-function-abi-variant" attribute.

This change is required to be able to provide instances of VFShape
that can be used to query the VFDatabase for the vectorization passes,
as such passes always require a positive value for the Vectorization Factor (VF)
needed by the vectorization process.

It is not possible to extract the value of VFShape.VF from the mangled
name of scalable vector functions, because it is encoded as
x. Therefore, the VFABI demangling function has been modified to
extract such information from the IR declaration of the vector
function, under the assumption that _all_ vectors in the signature of
the vector function have the same number of lanes. Such assumption is
valid because it is also assumed by the Vector Function ABI
specifications supported by the demangling function (x86, AArch64, and
LLVM internal one).

The unit tests that demangle scalable names have been modified by
adding the IR module that carries the declaration of the vector
function name being demangled.

In particular, the demangling function fails in the following cases:

  1. When the declaration of the scalable vector function is not present in the module.
  1. When the value of VFSHape.VF is not greater than 0.

Diff Detail

Event Timeline

fpetrogalli created this revision.Jan 23 2020, 12:25 PM

Putting this on hold - I just found a problem with the fuzzer. I'll ping you when ready to review.

Fix the failure in the fuzzer.

fpetrogalli edited the summary of this revision. (Show Details)Jan 23 2020, 1:14 PM

Another fix in the fuzzer: make sure that the parser doesn't crash if
the input llvm::Module *M is set to nullptr.

OK, I have been running the fuzzer for ~1h, no failures: ready to review!

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND
34158 frapet01  38  18 20.000t 549332  51564 R  99.7  0.4  57:56.06 vfabi-demangler

A few nits. Also, in the commit msg:

  • patche -> patch
  • which always set a positive value -> which is always set to a positive value?
  • mame ?

Maybe it would be good to add negative tests too? E.g. for when the module is missing.

llvm/lib/Analysis/VFABIDemangling.cpp
303

Why not if (auto RetTy = dyn_cast<VectorType>(Signature->getReturnType()))

332

Why not if (auto RetTy = dyn_cast<VectorType>(Signature->getReturnType())?

340

As per https://llvm.org/docs/CodingStandards.html#comment-formatting:

return {1 /*Min*/, false /*Scalable*/}; --> return {/*Min=*/1, /*Scalable=*/false};

llvm/tools/vfabi-demangle-fuzzer/vfabi-demangler-fuzzer.cpp
24–34
fpetrogalli marked 4 inline comments as done.

Address code review from @andwar (thank you!).

I have added 2 tests:

  1. Check that demangling of scalable names fails when the function

declaration is not present.

  1. Check that VLEN=0 is invalid for fixed-length vector names.
fpetrogalli edited the summary of this revision. (Show Details)Jan 27 2020, 11:11 AM

One [nit] (non-blocking), otherwise LGTM.

llvm/lib/Analysis/VFABIDemangling.cpp
471

This should never be hit because for VF = 0 tryParseVLEN returns an error and getECFromSignature returns at least 1.

I'm just wondering, maybe less would be more here?

fpetrogalli marked an inline comment as done.Jan 28 2020, 9:51 AM
fpetrogalli added inline comments.
llvm/lib/Analysis/VFABIDemangling.cpp
471

This should never be hit because for VF = 0 tryParseVLEN returns an error and getECFromSignature returns at least 1.

yes, but...

I'm just wondering, maybe less would be more here?

... I'd rather leave the assert to make sure that if someone adds code after the invocation of tryParseVLEN that sets VF to zero, they get a failure to make them aware of the problem.

Alternatively, we could make VF in VFShape a class that can only be a strictly positive number... but that seems to be over-killing...

Sorry for my delay. Some comments below.

llvm/include/llvm/Analysis/VectorUtils.h
170

Any reason one would not pass M here? If not, I would remove the default and maybe even make it required (via reference).

llvm/lib/Analysis/VFABIDemangling.cpp
302

Nit: using is not needed.

314

llvm::all_of

317

Nit: auto * and auto & wherever possible.

334

Let's be even more verbose here: ElementCount{...}

fpetrogalli marked an inline comment as done.Jan 28 2020, 12:40 PM
fpetrogalli added inline comments.
llvm/include/llvm/Analysis/VectorUtils.h
170

Could do - that will require changing all unit tests to provide a module though... are you happy for me to do that?

jdoerfert added inline comments.Jan 28 2020, 3:37 PM
llvm/include/llvm/Analysis/VectorUtils.h
170

I guess we can script it* so it should not be too bad work-wise. The question is, what do we want to achieve. If there is a use case that we want to support going forward which does not need the module, I'm fine with the pointer. If we are actually not anticipating such a use case we will just burden and confuse our future selves as we open up the possibility for all kinds of subtle bugs as someone forgot somewhere to pass a module, we should at least remove the default value.

  • Something like
for file in $(ag --files-with-matches "tryDemangleForVFABI")
do
  sed -i -e 's:tryDemangleForVFABI(\([^)]*\)):tryDemangleForVFABI(\1, nullptr):' ${file}
done

(UNTESTED)

fpetrogalli marked 5 inline comments as done.Jan 29 2020, 6:46 AM
fpetrogalli added inline comments.
llvm/include/llvm/Analysis/VectorUtils.h
170

On the long term I have a plan to require the Module, because I think to make sense to store the FunctionType of the variant instead of the pair VF and Scalable in VFShape. This is to support cases in which the vectorization factor is not uniform across the signature of the vector function - for example, where one parameter has two lanes and the other one four. Admittedly, such cases are not supported by the current vectorizer and cannot be represented via the VFABI rules, but we want to make VFShape generic enough so that it does not prevent adding such support if needed.

I'd rather not do this change now, because I think this will go beyond the scope of this patch, where all what I am trying to do is to make sure that VF is never zero when demangling the vector function.

So I have opted for passing const Module & to the demagler, and for adding an assertion right after having discovered the vector name to be used that makes sure that the name is present in the module itself. I'll finish up this soon and upload it.

jdoerfert accepted this revision.Jan 29 2020, 7:01 AM

LGTM, please address the nits mentioned here https://reviews.llvm.org/D73286#1845416

llvm/include/llvm/Analysis/VectorUtils.h
170

OK. Let's keep it for now this way.

This revision is now accepted and ready to land.Jan 29 2020, 7:01 AM

I have addressed the feedback from @jdoerfert.

In particular, the parser method now requires a Module instance. The
parser fails if the function declaration is not present in the
module. For this reason, I tweaked the test fixture in the unittest so
that the module and the function definition is present.

Notice that (as explained inthe comments in the code), for the sake of
testing, the signature of the vector function in the module is
relevant only for vector names that represent scalable functions.

fpetrogalli edited the summary of this revision. (Show Details)Jan 29 2020, 9:35 PM
fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli marked 4 inline comments as done.
This revision was automatically updated to reflect the committed changes.