This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add plain text (not DOT's digraph) dumps
ClosedPublic

Authored by a.elovikov on Feb 12 2021, 12:42 PM.

Details

Summary

I foresee two uses for this:

  1. It's easier to use those in debugger.
  2. Once we start implementing more VPlan-to-VPlan transformations (especially inner loop massaging stuff), using the vectorized LLVM IR as CHECK targets in LIT test would become too obscure. I can imagine that we'd want to CHECK against VPlan dumps after multiple transformations instead. That would be easier with plain text dumps than with DOT format.

Diff Detail

Event Timeline

a.elovikov created this revision.Feb 12 2021, 12:42 PM
a.elovikov requested review of this revision.Feb 12 2021, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 12:42 PM
fhahn added a comment.Mar 2 2021, 1:47 AM

Thanks for working on this! Could you split off the changes not directly related to printing in non-DOT mode (like respecting Indent in the individual print() implementations)? Then we can focus the review here on the details how to move to support non-DOT printing, which is very valuable IMO and should probably become the default in the long run.

llvm/lib/Transforms/Vectorize/VPlan.cpp
921

Could you split off the changes not directly related to printing in non-DOT mode (like respecting Indent in the individual print() implementations)?

Rebased on top of D97787.

fhahn added inline comments.Mar 9 2021, 1:39 PM
llvm/lib/Transforms/Vectorize/VPlan.cpp
53

I think having an option to just print the plans make sense, but as a first step, should we start with an option to just toggle between dot printing and regular printing for when using -debug?

864

Could you add a comment explaining that we first print the block and then split up/reconstruct the output with the .dot syntax>

fhahn added inline comments.Mar 9 2021, 1:40 PM
llvm/test/Transforms/LoopVectorize/icmp-uniforms.ll
41

Did the value here change because the plan gets printed later?

a.elovikov added inline comments.Mar 9 2021, 2:02 PM
llvm/lib/Transforms/Vectorize/VPlan.cpp
53

I preferred dedicated option for several reasons:

  1. Toggling is harder to implement, especially when caring about consistent interfaces. For example, I think that VPBasicBlock|VPValue|VPDef::print should be dedicated for text prints only (and VPBasicBlock can't even be printed as a digraph before this patch), so the only logical place for triggering would be operator<<(ostream&). On the other hand, there is no guarantees that none of the LLVM_DEBUG prints call the print method directly (and not via stream operator).
  1. I personally don't like using -debug/-debug-only for LIT testing purposes. Having a more limited/specific output is, IMO, preferable. I'm hoping that option(s) to print VPlan at a given place in the pipeline would be convenient tools for writing LIT tests. As such, I think having this option from the very first plain dump patch makes sense.

If going through the toggling approach, do you expect existing LIT tests (non-unittest) to continue using digraph output or be switched to plain dumps in the first patch? If the former, what would be your suggestion to test the plain dumps?

864

Sure, will update in the next patch set.

llvm/test/Transforms/LoopVectorize/icmp-uniforms.ll
41

I didn't study in details, but I'd expect it to be so. Is that something expected for you, or do you want me to study that change in details (my knowledge of the VPlan pipeline is still limited)?

  • Improve comments.
  • Don't add a new dump point via cl::opt. Instead, add cl::opt to toggle behavior (dot/plain dumps) for the LVP::printPlans method.
a.elovikov added inline comments.Mar 11 2021, 12:36 PM
llvm/test/Transforms/LoopVectorize/icmp-uniforms.ll
41

Yes, it gets changed in the middle of InnerLoopVectorizer::createVectorizedLoopSkeleton/InnerLoopVectorizer::createInductionResumeValues.

We seem to be using original LLVM IR value through VPlanIngredient when printing VPWidenIntOrFpInductionRecipe, so modifying original loop when creating the skeleton changes printing.

Thanks for the latest updates!

I just have a few remaining comments. After those, I think this is good to go. If there are any concerns with the direction by others, it would be great to hear them soon.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7829

I think it would make sense to flip things here, as in have the operator for raw_ostream use the non-dot style (because that's what most useful in debuggers too) and then have a Plan::printWithDot() or soemthing for the DOT logic.

llvm/lib/Transforms/Vectorize/VPlan.cpp
406

I think it would be good to separate the block names by a comma or something like that. You could either use join_items or ListSeparator from StringExtras.h

llvm/lib/Transforms/Vectorize/VPlan.h
1749

nit: comment, something like /// Print the plan to \p O.. Some for other instances.

llvm/test/Transforms/LoopVectorize/vplan-printing.ll
3

I think we should have at least a LIT test that also checks -vplan-print-in-dot-format=true. Perhaps this file would a good candidate to do so?

Address review comments.

llvm/lib/Transforms/Vectorize/VPlan.cpp
406

ListSeparator is great! Wasn't aware of it before.

fhahn accepted this revision.Mar 14 2021, 2:52 PM

LGTM, thanks! Please wait a day or 2 with committing, in case there are any remaining concerns with the direction. But I think it is a nice improvement to the user experience.

llvm/test/Transforms/LoopVectorize/vplan-dot-printing.ll
3

-prefer-inloop-reductions should not be needed

26

Nit: it would be good to at least one additional BB in the loop, to make sure the edges between blocks are still printed.

This revision is now accepted and ready to land.Mar 14 2021, 2:52 PM
a.elovikov added inline comments.Mar 14 2021, 7:29 PM
llvm/test/Transforms/LoopVectorize/vplan-dot-printing.ll
26

This functionality is being tested in unittests already, so I'm only using this file to target the toggle switch, not the digraph dump by itself.

This revision was landed with ongoing or failed builds.Mar 18 2021, 11:46 AM
This revision was automatically updated to reflect the committed changes.

Push a revert, it seems like it broke the link of clang itself:

FAILED: bin/clang-13
: && /usr/bin/clang++-8 -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fuse-ld=lld -Wl,--color-diagnostics   -Wl,--export-dynamic  -Wl,-O3 tools/clang/tools/driver/CMakeFiles/clang.dir/driver.cpp.o tools/clang/tools/driver/CMakeFiles/clang.dir/cc1_main.cpp.o tools/clang/tools/driver/CMakeFiles/clang.dir/cc1as_main.cpp.o tools/clang/tools/driver/CMakeFiles/clang.dir/cc1gen_reproducer_main.cpp.o -o bin/clang-13  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMX86CodeGen.a  lib/libLLVMX86AsmParser.a  lib/libLLVMX86Desc.a  lib/libLLVMX86Disassembler.a  lib/libLLVMX86Info.a  lib/libLLVMNVPTXCodeGen.a  lib/libLLVMNVPTXDesc.a  lib/libLLVMNVPTXInfo.a  lib/libLLVMAMDGPUCodeGen.a  lib/libLLVMAMDGPUAsmParser.a  lib/libLLVMAMDGPUDesc.a  lib/libLLVMAMDGPUDisassembler.a  lib/libLLVMAMDGPUInfo.a  lib/libLLVMAMDGPUUtils.a  lib/libLLVMAnalysis.a  lib/libLLVMCodeGen.a  lib/libLLVMCore.a  lib/libLLVMipo.a  lib/libLLVMAggressiveInstCombine.a  lib/libLLVMInstCombine.a  lib/libLLVMInstrumentation.a  lib/libLLVMMC.a  lib/libLLVMMCParser.a  lib/libLLVMObjCARCOpts.a  lib/libLLVMOption.a  lib/libLLVMScalarOpts.a  lib/libLLVMSupport.a  lib/libLLVMTransformUtils.a  lib/libLLVMVectorize.a  -lpthread  lib/libclangBasic.a  lib/libclangCodeGen.a  lib/libclangDriver.a  lib/libclangFrontend.a  lib/libclangFrontendTool.a  lib/libclangSerialization.a  lib/libLLVMCFGuard.a  lib/libLLVMAsmPrinter.a  lib/libLLVMDebugInfoDWARF.a  lib/libLLVMDebugInfoMSF.a  lib/libLLVMGlobalISel.a  lib/libLLVMSelectionDAG.a  lib/libLLVMMIRParser.a  lib/libLLVMAMDGPUDesc.a  lib/libLLVMAMDGPUInfo.a  lib/libLLVMAMDGPUUtils.a  lib/libLLVMMCDisassembler.a  lib/libclangCodeGen.a  lib/libLLVMCoverage.a  lib/libLLVMLTO.a  lib/libLLVMCodeGen.a  lib/libLLVMPasses.a  lib/libLLVMObjCARCOpts.a  lib/libLLVMTarget.a  lib/libLLVMCoroutines.a  lib/libLLVMipo.a  lib/libLLVMInstrumentation.a  lib/libLLVMVectorize.a  lib/libLLVMIRReader.a  lib/libLLVMAsmParser.a  lib/libLLVMScalarOpts.a  lib/libLLVMAggressiveInstCombine.a  lib/libLLVMInstCombine.a  lib/libLLVMBitWriter.a  lib/libLLVMLinker.a  lib/libLLVMExtensions.a  lib/libclangRewriteFrontend.a  lib/libclangARCMigrate.a  lib/libclangStaticAnalyzerFrontend.a  lib/libclangStaticAnalyzerCheckers.a  lib/libclangStaticAnalyzerCore.a  lib/libclangCrossTU.a  lib/libclangIndex.a  lib/libclangFrontend.a  lib/libclangDriver.a  lib/libLLVMOption.a  lib/libclangParse.a  lib/libclangSerialization.a  lib/libclangSema.a  lib/libclangAnalysis.a  lib/libclangASTMatchers.a  lib/libclangEdit.a  lib/libclangAST.a  lib/libLLVMFrontendOpenMP.a  lib/libLLVMTransformUtils.a  lib/libLLVMAnalysis.a  lib/libLLVMProfileData.a  lib/libLLVMObject.a  lib/libLLVMMCParser.a  lib/libLLVMBitReader.a  lib/libLLVMTextAPI.a  lib/libclangFormat.a  lib/libclangToolingInclusions.a  lib/libclangToolingCore.a  lib/libclangRewrite.a  lib/libclangLex.a  lib/libclangBasic.a  lib/libLLVMCore.a  lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMMC.a  lib/libLLVMBinaryFormat.a  lib/libLLVMDebugInfoCodeView.a  lib/libLLVMSupport.a  -lrt  -ldl  -lpthread  -lm  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a && :
ld.lld: error: undefined symbol: llvm::VPlan::printDOT(llvm::raw_ostream&) const
>>> referenced by LoopVectorize.cpp
>>>               LoopVectorize.cpp.o:(llvm::LoopVectorizationPlanner::printPlans(llvm::raw_ostream&)) in archive lib/libLLVMVectorize.a
 
ld.lld: error: undefined symbol: llvm::VPlan::print(llvm::raw_ostream&) const
>>> referenced by LoopVectorize.cpp
>>>               LoopVectorize.cpp.o:(llvm::LoopVectorizationPlanner::printPlans(llvm::raw_ostream&)) in archive lib/libLLVMVectorize.a
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Reading the logic a bit more, it looks to me that we need to put #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) inside LoopVectorizationPlanner::printPlans.

Sorry for the troubles and thanks for reverting that for me - I didn't receive notifications from buildbots for it. Yes, it's probably related to those #ifs. I'm starting a Release build to reproduce and create a fix.

mehdi_amini added inline comments.Mar 18 2021, 12:40 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7832

I think this ought to be guarded by #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) as well

It seems the code wasn't properly guarded before this change, and the proper fix would require changing too many places. I'd prefer to do it in separate patch.

I see two options:

  1. "Unguard" the print routines here and commit, work on the cleanup as the next patch.
  2. Delay this patch, develop cleanup first, rebase/reland this patch once cleanup is committed. Note that this patch doesn't make things worse - we had all operator<< accessible in release builds before it.

I'd prefer to go with the first one, but I can understand if some people would prefer two. What do you think?

I'd prefer to go with the first one, but I can understand if some people would prefer two. What do you think?

Whichever order works for you seems reasonable to me.

a.elovikov reopened this revision.Mar 18 2021, 1:30 PM
This revision is now accepted and ready to land.Mar 18 2021, 1:30 PM
This revision was landed with ongoing or failed builds.Mar 19 2021, 10:50 AM
This revision was automatically updated to reflect the committed changes.
bmahjour removed a subscriber: bmahjour.Mar 19 2021, 5:43 PM