[VP,Integer,#2] ExpandVectorPredication pass

[simoll]

Summary

This patch implements expansion of llvm.vp.* intrinsics (https://llvm.org/docs/LangRef.html#vector-predication-intrinsics).

VP expansion is required for targets that do not implement VP code generation. Since expansion is controllable with TTI, targets can switch on the VP intrinsics they do support in their backend offering a smooth transition strategy for VP code generation (VE, RISC-V V, ARM SVE, AVX512, ..).

ExpandVectorPredication

The ExpandVectorPredication pass that folds the EVL parameter into the vector mask and/or lowers a VP intrinsics to a standard IR instruction.

[frasercrmck]

Aside from my nitpicks, I was wondering if it's possible to add tests for the other strategies. Even if that involved a strategy that you could force on the command-line, since I know we don't have real TTIs just now. It would be nice to see that the pass behaves in the various ways it's meant to.

llvm/lib/CodeGen/ExpandVectorPredication.cpp

313

stray lower-case l in EVL (or should it be %evl?)

318

typo (reassess), also preferably with a capital R to match the style of the other comments

352

typo: strictly

365

Comment trails off here.

387

You shouldn't need the parentheses in these switch cases, also the formatting with the breaks looks a little off to me.

[simoll]

Linking this to the SelectionDAG patch: D91441

[simoll]

In D78203#2355876, @frasercrmck wrote:

Aside from my nitpicks, I was wondering if it's possible to add tests for the other strategies. Even if that involved a strategy that you could force on the command-line, since I know we don't have real TTIs just now. It would be nice to see that the pass behaves in the various ways it's meant to.

I like that idea! I'll add some cl::opts to override the target strategy for tests. Thx :)

[simoll]

• Added override options to override the expansion strategy.
• Added expansion tests for fixed and scalable types and all possible expansion strategies.
• Rebased.

[simoll]

llvm/include/llvm/IR/Intrinsics.td

1302 ↗

(On Diff #306066)

FYI I'll commit the attribute changes to the VP intrinsics separately.

[simoll]

Rebased onto "speculatable" VP intrinsic patch.

[simoll]

Ping. Where do we stand with this patch, can we LGTM or are there comments?

[ctetreau]

This all seams fairly reasonable to me, just some minor nitpicks. I'm not confident in giving this the final approval, but hopefully this can serve as a ping to those who are.

llvm/include/llvm/Analysis/TargetTransformInfo.h

1383

BIKESHED: "doNothing" sounds like a nop, not a test

llvm/lib/CodeGen/ExpandVectorPredication.cpp

138

This could conceivably work for scalable vectors after https://lists.llvm.org/pipermail/llvm-dev/2021-January/147943.html is merged

226–229

please name the type.

249

please name the type

255

please name types where the typename does not appear on the RHS.

303–304

please name the type

323–324

should these get undefined after the include?

[simoll]

Addressed @ctetreau 's comments. Thx!

[simoll]

Modernize tests, replace undef -> poison

[simoll]

llvm/lib/CodeGen/ExpandVectorPredication.cpp

138

Yep. We can switch to stepvector once its merged.

323–324

These get undefined in the included file.

[frasercrmck]

Looks good to me in general, but I'm similarly not comfortable giving the final approval.

llvm/lib/CodeGen/ExpandVectorPredication.cpp

227

super nit: I'm not sure FirstOp and SndOp reads very well. I see that MemoryBuiltins uses FstParam and SndParam, but more common would be something like Op0 and Op1?

[simoll]

NFC

• Addressed super nit
• Formatting
• Rebased

[craig.topper]

llvm/lib/CodeGen/ExpandVectorPredication.cpp

68

Is this supposed to be a std::string& ?

76

Would !EVLTransformOverride.empty() || !MaskTransformOverride.empty() be better?

91

return ConstVec && ConstVec->isAllOnesValues();

96

No message

166

Is this supposed to be a doxygen comment? It only uses 2 slashes

175

Does this comment go to the constructor? the \p references don't apply to this function. But it's not a doxygen comment.

186

Drop curly braces.

327

Can this use Instruction::isBinaryOp?

393

Drop curly braces

401

This might read better as

"\n::::Transforming " << Worklist.size() << " Instructions"

Then it's obvious the number in parens is a count.

llvm/lib/IR/IntrinsicInst.cpp

201

getValue() will assert itself won't it?

214

Same

[rogfer01]

llvm/lib/CodeGen/ExpandVectorPredication.cpp

72

I think this should be VPINTERNAL_VPLEGAL_CASES

llvm/test/CodeGen/Generic/expand-vp.ll

2

Just a suggestion: llvm/utils/update_test_checks.py can reduce the cost of maintaining this test (I understand we'll have to extend it when VP FP lands).

[simoll]

llvm/test/CodeGen/Generic/expand-vp.ll

2

How about we switch to an auto-generated test once caching is in and the code has stabilized?
Without caching the output is very verbose.. also auto-generated tests are really sentinels for change not actually tests

[simoll]

• NFC
• worked through reviewers' comments (thx!)
• rebased

[craig.topper]

llvm/lib/CodeGen/ExpandVectorPredication.cpp

180

Why int32_t here? The caller uses unsigned and LLVM doesn't usually use specific sized types for integers unless the bit width is important.

[rogfer01]

I understand this is a legacy pass manager. Would it make sense to adapt it to the New PM as well?

llvm/test/CodeGen/Generic/expand-vp.ll

2

Yep. Agreed.

[simoll]

In D78203#2707363, @rogfer01 wrote:

I understand this is a legacy pass manager. Would it make sense to adapt it to the New PM as well?

AFAIK SelectionDAG lives and dies with the old pm.
In any case, there is a wrapper pass for the new pm in this patch modeled after the one for reduction expansion. It's just not being used atm. I guess that's good enough?

llvm/include/llvm/CodeGen/ExpandVectorPredication.h

17–22

Wrapper for the new pm

[rogfer01]

I don't think there is anything controversial left to address (and if found we can always fix it later). In the light of the scenarios that will be enabled by this pass I believe there is no reason not to land this change.

[aprantl]

This commit may have broken the -DLLVM_ENABLE_MODULES=1 build. See http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31603/consoleFull#2136199809a1ca8a51-895e-46c6-af87-ce24fa4cd561

FAILED: lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/ExpandVectorPredication.cpp.o 
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/CodeGen -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/CodeGen -Iinclude -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include -Wdocumentation -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -fmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache -fcxx-modules -Xclang -fmodules-local-submodule-visibility -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/ExpandVectorPredication.cpp.o -MF lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/ExpandVectorPredication.cpp.o.d -o lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/ExpandVectorPredication.cpp.o -c /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/CodeGen/ExpandVectorPredication.cpp
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/CodeGen/ExpandVectorPredication.cpp:461:30: error: redefinition of 'run'
ExpandVectorPredicationPass::run(Function &F, FunctionAnalysisManager &AM) {
                             ^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/CodeGen/MachinePassRegistry.def:106:1: note: previous definition is here
DUMMY_FUNCTION_PASS("expandvp", ExpandVectorPredicationPass, ())
^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/CodeGen/CodeGenPassBuilder.h:68:23: note: expanded from macro 'DUMMY_FUNCTION_PASS'
    PreservedAnalyses run(Function &, FunctionAnalysisManager &) {             \
                      ^
1 error generated.

Can you please investigate?

[aprantl]

Since it's already late on Friday I took the liberty to revert the commit so the bots can recover and you can investigate the failure at your own pace. Let me know if I can help with the fix!

[simoll]

In D78203#2730888, @aprantl wrote:

Since it's already late on Friday I took the liberty to revert the commit so the bots can recover and you can investigate the failure at your own pace. Let me know if I can help with the fix!

Thanks for taking action! I'll look into how include/llvm/CodeGen/MachinePassRegistry.def actually works.. i suppose moving the entry from DUMMY_FUNCTION_PASS to FUNCTION_PASS should fix this (will reproduce and fix locally). Never had a revert, what is the usual procedure to recommit after revert? (update/new diff, review, or just commit with the fix?)

[aprantl]

In D78203#2732842, @simoll wrote:

In D78203#2730888, @aprantl wrote:

Since it's already late on Friday I took the liberty to revert the commit so the bots can recover and you can investigate the failure at your own pace. Let me know if I can help with the fix!

Thanks for taking action! I'll look into how include/llvm/CodeGen/MachinePassRegistry.def actually works.. i suppose moving the entry from DUMMY_FUNCTION_PASS to FUNCTION_PASS should fix this (will reproduce and fix locally). Never had a revert, what is the usual procedure to recommit after revert? (update/new diff, review, or just commit with the fix?)

You can just re-commit with a comment in the commit message explaining how the re-commit differs from the original one. Unless you make substantial changes or you are unsure about your solution, there is no need to re-review the patch.