This is an archive of the discontinued LLVM Phabricator instance.

[SLP][AArch64] Incorrectly estimated intrinsic as a function call
ClosedPublic

Authored by dtemirbulatov on Dec 20 2022, 5:36 AM.

Details

Summary

I found that currently we incorrectly assume intrinsic as a function call and it prevents us from the opportunity to vectorize. On Aarch64 Cortex-A53 we think that llvm.fmuladd.f64 is a function call which is wrong, but we could ask the backend about the cost of such an intrinsic to determine on how it ends instruction(s)/function call. This is one of the reasons why LNT's matmul_f64_4x4.c is not vectorized on AArch64.

Diff Detail

Event Timeline

dtemirbulatov created this revision.Dec 20 2022, 5:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 5:36 AM
dtemirbulatov requested review of this revision.Dec 20 2022, 5:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 5:36 AM
dtemirbulatov retitled this revision from [SLP][AArch64] incorrectly estimated intrinsic as a function call to [SLP][AArch64] Incorrectly estimated intrinsic as a function call.Dec 20 2022, 5:42 AM
ABataev added inline comments.Dec 20 2022, 6:39 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7570–7591

Can you tranform all this code to lambda?

7571–7572
if (auto *II = dyn_cast<IntrinsicInst>(&*PrevInstIt))
7574–7576

Sink this code to the else substatement.

7578–7580

Braces

7586–7591

Do we have a btter way to check if the intrinsic is lowered as an instruction rather than the call?

NoCallIntrinsic = IntrCost < CallCost;
llvm/test/Transforms/SLPVectorizer/AArch64/fmulladd.ll
1

Need to precommit the test

3–4

I assume this can be removed

9

cleanup attrs, I assume they are not required.

75

Remove attribute reference

dtemirbulatov marked 8 inline comments as done.

Rebased, Addressed remarks.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7586–7591

No, it looks to me the only way. We could duplicate the code from getTypeBasedIntrinsicInstrCost() and ask about legality of an operation from a target, but it would be too much to duplicate. Also, we could hardcode minimal call cost, for example 10, but it would look incorrect to me too.

ABataev added inline comments.Jan 3 2023, 5:30 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7572–7574

You can drop braces here

7574

No need for else after if with the return substatemenet.

dtemirbulatov marked 2 inline comments as done.

Addressed comments.

ABataev added inline comments.Jan 3 2023, 6:00 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7576

Do you still need this cast to get the list of arguments?

dtemirbulatov added inline comments.Jan 3 2023, 6:32 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7576

yes, in order to get correctly prepared ICA. Otherwise sometime we could get incorrect cost or compile time error if we provide empty set of arguments.

ABataev added inline comments.Jan 3 2023, 6:36 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7576

II is already an IntrinsicInst, which has CallInst as the base class. I mean, you don't need to do another cast here, you can use II instead of CI

Removed unnecessary cast to CallInst.

dtemirbulatov marked an inline comment as done.Jan 3 2023, 7:27 AM
ABataev accepted this revision.Jan 3 2023, 8:40 AM

LG with a nit

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
7596

Remove extra parens around isa call.

This revision is now accepted and ready to land.Jan 3 2023, 8:40 AM