[VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names #67430

JolantaJensen · 2023-09-26T13:45:12Z

[VFABI] Refactor tryDemangleForVFABI to use only the Vector ABI mangled names
        and the call's argument types to construct the VFInfo.

Currently the tryDemangleForVFABI is incorrectly making assumptions that when
the mangled name has __LLVM__ set as ISA and _vlen_ is set to scalable/VLA
the module can be queried to identify the correct VF. This works currently only
by chance rather than by design and limits the ability to maximise the use for
the Vector ABI. This behaviour is changed by
https://github.com/llvm/llvm-project/pull/66656 and
https://github.com/llvm/llvm-project/pull/67308
and means demangling can be implemented as expected based solely on the rules
as documented within the VFABI.

JolantaJensen · 2023-09-26T13:48:45Z

llvm/tools/vfabi-demangle-fuzzer/vfabi-demangler-fuzzer.cpp

@@ -27,15 +27,17 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
  // present. We need to make sure we can even invoke
  // `getOrInsertFunction` because such method asserts on strings with
  // zeroes.


I'm not sure about this test. The comment is definitely wrong now but does the test still make sense?

paulwalker-arm · 2023-09-28T17:10:54Z

llvm/lib/Analysis/VFABIDemangling.cpp

      return std::nullopt;
-    const ElementCount EC = getECFromSignature(F->getFunctionType());


What about keeping getECFromSignature but passing it the ISA and the FunctionType of the CallInst?

But to get the FunctionType, I need the Function itself, and to get the Function I need the Module. And what about an Instruction that isn't a CallInstruction and won't give me a Function? Or do you mean passing FunctionType instead of the vector with Operand Types? Or passing the Instruction itself as argument?

paulwalker-arm · 2023-09-28T17:13:10Z

llvm/lib/Analysis/VectorUtils.cpp

    LLVM_DEBUG(dbgs() << "VFABI: adding mapping '" << S << "'\n");
-    std::optional<VFInfo> Info =
-        VFABI::tryDemangleForVFABI(S, *(CI.getModule()));
+    std::optional<VFInfo> Info = VFABI::tryDemangleForVFABI(S, OpTys);


You previously identified the operands might not be enough so perhaps just pass in the call instruction itself rather than spend time pulling the operands into a separate vector.

…ed names and the call's argument types to construct the VFInfo. Currently the tryDemangleForVFABI is incorrectly making assumptions that when the mangled name has __LLVM__ set as ISA and _vlen_ is set to scalable/VLA the module can be queried to identify the correct VF. This works currently only by chance rather than by design and limits the ability to maximise the use for the Vector ABI. This behaviour is changed by llvm#66656 and llvm#67308 and means demangling can be implemented as expected based solely on the rules as documented within the VFABI.

JolantaJensen · 2023-10-05T18:05:02Z

llvm/tools/vfabi-demangle-fuzzer/vfabi-demangler-fuzzer.cpp

+    FunctionType *FTy =
+        FunctionType::get(Type::getVoidTy(M->getContext()), false);
+    FunctionCallee F = M->getOrInsertFunction(MangledName, FTy);
+    // Fake the arguments to the CallInst.
+    SmallVector<Value *> Args;
+    for (Type *ParamTy : FTy->params()) {
+      Args.push_back(Constant::getNullValue(ParamTy));
+    }
+    std::unique_ptr<CallInst> CI(CallInst::Create(F, Args));
+    const auto Info = VFABI::tryDemangleForVFABI(MangledName, *(CI.get()));

-  // Do not optimize away the return value. Inspired by
-  // https://github.com/google/benchmark/blob/main/include/benchmark/benchmark.h#L307-L345
-  asm volatile("" : : "r,m"(Info) : "memory");
+    // Do not optimize away the return value. Inspired by
+    // https://github.com/google/benchmark/blob/main/include/benchmark/benchmark.h#L307-L345
+    asm volatile("" : : "r,m"(Info) : "memory");

-  return 0;
+    return 0;


Here, I'm only returning a value if the if condition is true but it does not make a sense to me to try to call VFABI::tryDemangleForVFABI if we do not have a CI. Grateful for suggestions how to rewrite this part.

paulwalker-arm

Given my current comments I've not reviewed the tools and unittests changes but broadly this is looking like a nice cleanup, you just need to firm up which type is used to calculate the ElemetCount.

paulwalker-arm · 2023-10-12T10:54:52Z

llvm/include/llvm/Analysis/VectorUtils.h

-/// name. At the moment, this parameter is needed only to retrieve the
-/// Vectorization Factor of scalable vector functions from their
-/// respective IR declarations.
+/// \param CI -> the call instruction.


It's worth being a little more verbose here and say something like "the call instruction to the scalar function to which MangledName is being linked and whose return and parameter types are required to calculate the expected ElementCount for the matching vector function".

paulwalker-arm · 2023-10-12T11:27:12Z

llvm/lib/Analysis/VFABIDemangling.cpp

+    assert(verifyAllVectorsHaveSameWidth(Signature) &&
+           "Invalid vector signature.");


I'm not sure this makes much sense because isn't CI the call to the scalar function? I'm thinking verifyAllVectorsHaveSameWidth only existed due to the bogus way EC was previously calculated.

paulwalker-arm · 2023-10-12T11:36:57Z

llvm/lib/Analysis/VFABIDemangling.cpp

+    ArrayRef<Type *> ParamTys = Signature->params();
+    if (ParamTys.empty()) {
+      return ParseRet::Error;
+    }


It's likely more complicated than this because I believe we support the case where there's a return value but no parameters.

Also, now I can see the code, I think I've given you an incomplete steer because in order to support things like "uniform" and "linear" we cannot pick just any parameter, we first need to know which of the parameters are required to be vector types. This means tryParseVLEN likely needs to be called last after we construct the VFParameter's.

paulwalker-arm · 2023-11-28T13:37:01Z

Superseded by #72260 that landed last week.

JolantaJensen requested review from paulwalker-arm and mgabka September 26, 2023 13:45

llvmbot added llvm:analysis llvm:transforms labels Sep 26, 2023

JolantaJensen commented Sep 26, 2023

View reviewed changes

paulwalker-arm reviewed Sep 28, 2023

View reviewed changes

JolantaJensen added 2 commits October 5, 2023 17:02

Addressed review comments.

4ab245f

JolantaJensen force-pushed the Refactor-tryDemangleForVFABI-to-use-only-the-Vector-ABI-mangled-names branch from a0754c9 to 4ab245f Compare October 5, 2023 17:50

JolantaJensen commented Oct 5, 2023

View reviewed changes

Correcting the fuzzer test

f2c3c9c

JolantaJensen changed the title ~~Refactor try demangle for vfabi to use only the vector abi mangled names~~ [VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names Oct 11, 2023

paulwalker-arm reviewed Oct 12, 2023

View reviewed changes

huntergr-arm mentioned this pull request Nov 10, 2023

Assertion `Info && "Invalid name for a VFABI variant."' failed. #71892

Closed

paulwalker-arm closed this Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names #67430

[VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names #67430

JolantaJensen commented Sep 26, 2023 •

edited

Loading

JolantaJensen Sep 26, 2023

JolantaJensen Oct 9, 2023

paulwalker-arm Sep 28, 2023

JolantaJensen Sep 29, 2023

JolantaJensen Oct 9, 2023

paulwalker-arm Sep 28, 2023

JolantaJensen Oct 5, 2023

JolantaJensen Oct 9, 2023

paulwalker-arm left a comment

paulwalker-arm Oct 12, 2023

paulwalker-arm Oct 12, 2023

paulwalker-arm Oct 12, 2023 •

edited

Loading

paulwalker-arm commented Nov 28, 2023

		return std::nullopt;
		const ElementCount EC = getECFromSignature(F->getFunctionType());

		assert(verifyAllVectorsHaveSameWidth(Signature) &&
		"Invalid vector signature.");

[VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names #67430

[VFABI] Refactor try demangle for vfabi to use only the vector abi mangled names #67430

Conversation

JolantaJensen commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulwalker-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulwalker-arm Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

paulwalker-arm commented Nov 28, 2023

JolantaJensen commented Sep 26, 2023 •

edited

Loading

paulwalker-arm Oct 12, 2023 •

edited

Loading