This is an archive of the discontinued LLVM Phabricator instance.

[VP,Integer,#2] ExpandVectorPredication pass
ClosedPublic

Authored by simoll on Apr 15 2020, 6:38 AM.

Details

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.

Diff Detail

Event Timeline

simoll created this revision.Apr 15 2020, 6:38 AM

Since this is bound to come up: Yes, there is no expansion for scalable types yet because i could not find a target-agnostic way to generate a step vector. I only found this seemingly abandoned patch: https://reviews.llvm.org/D47774. Any suggestion for a (portable) evl-to-mask lowering or patches for stepvector are appreciated.

lenary removed a subscriber: lenary.Apr 15 2020, 7:48 AM
vkmr added a subscriber: vkmr.Apr 16 2020, 3:56 AM
arsenm added inline comments.Apr 16 2020, 3:37 PM
llvm/include/llvm/CodeGen/ExpandVectorPredication.h
2

C++ mode comment

llvm/lib/CodeGen/ExpandVectorPredication.cpp
198–199

Shouldn't be true for FP divisions

272–273

Combine into one LLVM_DEBUG

simoll updated this revision to Diff 258250.Apr 17 2020, 1:27 AM
simoll marked 4 inline comments as done.
  • fixed sanitizeStrategy
  • arsenm's comments (thx!)
  • camelCaseForFunctionNames
  • rebased
simoll added inline comments.Apr 17 2020, 1:28 AM
llvm/lib/CodeGen/ExpandVectorPredication.cpp
198–199

Yep. At least not before we have vector-predicated, constrained fdiv.

reames resigned from this revision.May 22 2020, 3:26 PM
simoll planned changes to this revision.May 26 2020, 5:23 AM

This will use llvm.get.active.lane.mask to expand VP intrinsics for scalable vectors on targets that do not support the %evl parameter (D79100).

simoll updated this revision to Diff 268204.Jun 3 2020, 8:17 AM
simoll edited the summary of this revision. (Show Details)
  • Use llvm.get.active.lane.mask to fold the %evl parameter into the vector mask parameter for scalable VP ops.
  • Never discard %evl if the operation may cause UB (eg for sdiv, udiv).
  • Added tests for VP ops with scalable types.
craig.topper added inline comments.
llvm/include/llvm/IR/PredicatedInst.h
35

Can this inherit from Instruction? Its a little surprising that it has Instruction in its name but has to be casted to Instruction.

51

Can we move this above copyIRFlags to keep all the deleted things together and away from code that does stuff.

llvm/lib/CodeGen/ExpandVectorPredication.cpp
46

Should this start with lower case 'i'?

71

Add /* Vector */ comment to the true.

77

Save the register bit width instead of querying it again?

99

This isn't safe for sdiv/srem is it? INT_MIN/-1 is overflow and will trap.

248

Do we need the nullptr here? I had to lookup what argument that is.

llvm/lib/IR/PredicatedInst.cpp
29

I think we prefer "auto *" for pointers?

simoll planned changes to this revision.Jun 4 2020, 1:33 AM
simoll marked 12 inline comments as done.

@craig.topper Thanks for your comments! I'll update shortly.

llvm/include/llvm/IR/PredicatedInst.h
35

Conceptually, PredicatedInstruction is a super class of Instruction: An Instruction is a PredicatedInstruction with the mask/evl parameters hard-wired to "all lanes true". That's why we can't make Instruction an ancestor of this one.
I'll clarify this with a comment.

llvm/lib/CodeGen/ExpandVectorPredication.cpp
71

This really should have its own enum.

77

Moved this function in to the CachingVPExpander class and added a lane bits cache there.

99

Good point. I'll fix this.

llvm/lib/IR/PredicatedInst.cpp
29

Turns out we do. Fixed.

simoll updated this revision to Diff 268387.Jun 4 2020, 1:50 AM
simoll marked 4 inline comments as done.
simoll added a reviewer: craig.topper.

Addressed @craig.topper 's comments

simoll marked an inline comment as done.Jun 4 2020, 2:11 AM
simoll added inline comments.
llvm/lib/IR/IntrinsicInst.cpp
304–307

This unrelated change fixes a bug (ParamFactor does not make sense as a return value here and is not initialized!). I'll push a fix in a separate commit along with a test for this case.

simoll updated this revision to Diff 268436.Jun 4 2020, 5:29 AM

Fixed VPIntrinsic::canIgnoreVectorLength separately (a0dfdda4e5e8) and rebased onto that patch.

There are still more "auto" variables that are pointers in this patch.

llvm/include/llvm/IR/PredicatedInst.h
35

But a VPIntrinsic is also an Instruction isn't it? At least the CallInst for the vp intrinsic is an Instruction.

simoll planned changes to this revision.Jun 5 2020, 1:17 AM
simoll marked an inline comment as done.

Will fix remaining auto* cases.

llvm/include/llvm/IR/PredicatedInst.h
35

The whole reason for having the PredicatedInstruction class is that we want to add a new perspective on the IR where all instructions may be predicated (through a mask and evl). There are no VPIntrinsics in that perspective.

In the standard IR perspective, using the Instruction type, VPIntrinsics are just regular CallInsts calling intrinsics.

simoll updated this revision to Diff 268724.Jun 5 2020, 3:00 AM
  • Use auto*
  • Rebased
craig.topper added inline comments.Jun 5 2020, 3:13 PM
llvm/include/llvm/IR/PredicatedInst.h
35

I was just wanting to get rid of the need to cast to Instruction in copyIRFlags and getParent and replaceOperation etc. Everything this class represents must be an Instruction whether its predicated or not. No type safety comes from this class inheriting from User instead of Instruction since a CallInst to a VP intrinsic is also an Instruction.

simoll planned changes to this revision.Jun 8 2020, 12:56 AM
simoll marked an inline comment as done.

Will rephrase this patch with VPIntrinsic instead of PredicatedInstruction since the purpose of the latter will only become clear with generalized pattern matching/lifting optimizations to VP.

llvm/include/llvm/IR/PredicatedInst.h
35

That's exactly the point: a PredicatedInstruction cannot implicitly cast to an Instruction because that would be an implicit change of perspective.

simoll updated this revision to Diff 269123.Jun 8 2020, 1:54 AM
  • Removed PredicatedInstruction, which isn't strictly necessary for this patch.
  • Rebased.
simoll edited the summary of this revision. (Show Details)Jun 8 2020, 2:01 AM
khchen added a subscriber: khchen.Sep 5 2020, 12:09 AM
simoll updated this revision to Diff 296121.Oct 5 2020, 2:49 AM
  • adapted to get.active.mask changes (ult instead of ule)
  • rebased
simoll updated this revision to Diff 296136.Oct 5 2020, 3:17 AM

fixed for privatized ElementCount members.

Any more feedback for this patch?

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.

Linking this to the SelectionDAG patch: D91441

simoll planned changes to this revision.Nov 16 2020, 5:23 AM
simoll marked 3 inline comments as done.

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 updated this revision to Diff 306066.Nov 18 2020, 5:22 AM
simoll marked 2 inline comments as done.
  • Added override options to override the expansion strategy.
  • Added expansion tests for fixed and scalable types and all possible expansion strategies.
  • Rebased.
simoll added inline comments.Nov 18 2020, 5:23 AM
llvm/include/llvm/IR/Intrinsics.td
1302 ↗(On Diff #306066)

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

simoll updated this revision to Diff 306142.Nov 18 2020, 9:52 AM

Rebased onto "speculatable" VP intrinsic patch.

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

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
1221

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 updated this revision to Diff 328094.Mar 4 2021, 2:40 AM
simoll marked 6 inline comments as done.

Addressed @ctetreau 's comments. Thx!

simoll updated this revision to Diff 328142.Mar 4 2021, 5:49 AM

Modernize tests, replace undef -> poison

simoll added inline comments.Mar 4 2021, 7:03 AM
llvm/lib/CodeGen/ExpandVectorPredication.cpp
138

Yep. We can switch to stepvector once its merged.

323–324

These get undefined in the included file.

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 updated this revision to Diff 334163.Mar 30 2021, 7:38 AM
simoll marked an inline comment as done.

NFC

  • Addressed super nit
  • Formatting
  • Rebased
craig.topper added inline comments.Apr 13 2021, 9:02 AM
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 added inline comments.Apr 15 2021, 12:53 AM
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 marked 13 inline comments as done.Apr 19 2021, 2:21 AM
simoll added inline comments.
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 updated this revision to Diff 338510.Apr 19 2021, 7:05 AM
  • NFC
  • worked through reviewers' comments (thx!)
  • rebased
craig.topper added inline comments.Apr 19 2021, 12:52 PM
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.

simoll updated this revision to Diff 338747.Apr 20 2021, 1:09 AM
simoll marked an inline comment as done.

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 added a comment.EditedApr 23 2021, 2:56 AM

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 accepted this revision.Apr 29 2021, 11:32 PM

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.

This revision is now accepted and ready to land.Apr 29 2021, 11:32 PM
This revision was landed with ongoing or failed builds.Apr 30 2021, 6:48 AM
This revision was automatically updated to reflect the committed changes.

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?

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!

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?)

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.