This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower VLEFF/VLSEGFF SDNodes to MachineInstrs with VL outputs.
ClosedPublic

Authored by fakepaper56 on Jun 1 2022, 10:13 AM.

Details

Summary

The patch is a replacement of D125199. PseudoReadVL with vtype has worry for
computing same vtypes of VLEFF/VLSEGFF in two different places, DAGToDAG and
InsertVSETVLI. VLEFF/VLSEGFF MI with VL output still could provide the vtype of
VLEFF/VLSEGFF to the users of its VL.

The patch names the new pseudo as original VLEFF/VLSEGFF name suffixed "_VL" and
expand them in RISCVInsertVSETVLI pass.

This patch also reverts commit 4537aae0d57e17c217c192d8977012ba475b130c,
"[RISCV] Make PseudoReadVL have the vtypes of the corresponding VLEFF/VLSEGFF.".

Diff Detail

Event Timeline

fakepaper56 created this revision.Jun 1 2022, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 10:13 AM
fakepaper56 requested review of this revision.Jun 1 2022, 10:13 AM

Remove test cases of "[RISCV] Make PseudoReadVL have the vtypes of the corresponding VLEFF/VLSEGFF."

In general, I like this patch more than the alternative. However, I also have to acknowledge that this is mostly just shifting complexity from one place to another. I'm curious to know if others think this is the right tradeoff.

Once we've settled on the approach, I think a few bits of this can be separated out into NFCs to simplify the review, but let's leave that for the moment. What's here is a pretty good view of the overall impact.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

Not sure how much value we have here from having the generic bit field for this over a static method which switches on the opcodes. This really only applies to the FF variants.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1424

Did you play with what happens if you just set the second destination register to VL?

If that works, then we'd avoid the need for the second set of pseudos. Not sure it does, but maybe?

1425

It looks like this block of code is just copying all the operands right? If so, simpler to write it that way.

craig.topper added inline comments.Jun 2 2022, 9:39 PM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1424

The register class wouldn't match, so I doubt it would work.

Might be able to use an OptionalDef though?

fakepaper56 added inline comments.Jun 3 2022, 3:32 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

How about use getNumExplicitDefs() == 2 to check whether a MI is VLEFF/VLSEGFF?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1424

Just replacing CurDAG->getRegister(RISCV::VL, Subtarget->getXLenVT()) with ReadVL causes ICE about legalization. I don't really understand OptionalDef yet, but I have some questions about the method.

First, the method seems to break the SSA form of VL and we need to know which modifies the VL right?
Second, Is possible that the use of VL be moved behind the VLEFF/VLSEGFF MI?

reames added inline comments.Jun 3 2022, 8:04 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

If it were hidden in a reasonable named helper function, maybe. I'm concerned about how many other instructions might have two explicit defs, and in particular, how easy it would be to add an unrelated one without knowing this check existed.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1424

Two other options for you to consider.

Could we set the second def to X0? Conceptually, X0 is a constant and thus write discarding register, so this would be a "correct" GPR def.

Could we set the second def to NoRegister? We have precedent for this in a couple other places (e.g. mask reg).

Basically, I'm looking for something we can sensibly set it to which allows it to be a nop after we expand the separate copy, and yet leaves us with one family of psuedos.

Simplify code about expanding VLEFF/VLSEGFF.

fakepaper56 marked an inline comment as done.Jun 3 2022, 8:15 AM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1425

Done.

fakepaper56 marked an inline comment as done.Jun 3 2022, 8:47 AM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

Yes, there is a bunch of MI has 2 explicit defines but not VLEFF/VLSEGFF. Executing sed '/RISCVInsts\[\] =/,/^}/!d' <llvm-build-dir>/lib/Target/RISCV/RISCVGenInstrInfo.inc | awk 'index($4, 2)' could see all the opcode having 2 explicit defines .
I am sorry not do the experiment first.

Address reames's comment to use exactly one set pseudo of VLEFF/VLSEGFF. Thank that reames's comments make the patch cleaner.

fakepaper56 marked an inline comment as done.Jun 5 2022, 2:31 AM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

I found we could use hasSEWOp(TsFlags) && MI->mayLoad() && MI->getNumExplicitDefs() == 2 to replace the hasVLOutput(). Or maybe we just enumerate all the VLEFF/VLSEGFF instructions into a switch case like isFaultFirstLoad() in D126227.

reames added a comment.Jun 6 2022, 7:41 AM

This is looking pretty reasonable to me. Another rev or two, and we're probably good to go.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
173

The isFaultFirstLoad function from the other patch was what I'd originally suggested. I would mildly prefer that, but your proposed last condition in a well named function would be fine too.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

Unless I'm missing something, the need for separate FF pseudos disappears once you remove the VLOperand flag doesn't it?

frasercrmck added inline comments.Jun 6 2022, 7:46 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
9

Should maybe update this comment as this patch feels to me a like a new distinct step that this pass does (other than the recent pre/post passes and optimizations we've added recently and haven't listed here)

fakepaper56 added inline comments.Jun 6 2022, 7:55 PM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
9

You are right. I will add some comment there.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

The VLOperand you mentioned is HasVLOutput in my code? I think we should still use separate FF pseudos, since the output of VLEFF/VLSEGFF needs two explicit defines.

craig.topper added inline comments.Jun 6 2022, 7:59 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

Did we rule out making it an OptionalDef and setting the def to NoRegister?

fakepaper56 added inline comments.Jun 6 2022, 9:40 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

Sorry I forgot OptionalDef, I will try to use it.

fakepaper56 added inline comments.Jun 7 2022, 5:56 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

I think OptionalDef is only for physical register, the case needs virtual register fit into the output. There is another solution to merge VPseudoUSLoad and VPseudoUSLoadFF is to make RISCVVLE and RISCVVLSEG have two explicit defines, but I think the way is too aggressive.

reames requested changes to this revision.Jun 8 2022, 8:07 AM

Please refresh with prior comments addressed so I can LGTM.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
744

Let's drop this point for now. I'm fine with this landing with this particular point unaddressed, we can revisit in tree.

This revision now requires changes to proceed.Jun 8 2022, 8:07 AM

Add comments for RISCVInsertVSETVLI and uses MI.getNumExplicitDefs() == 2 && MI.modifiesRegister(RISCV::VL) && !MI.isInlineAsm() to identify VLEFF/VLSEGFF instead of adding new TSFlag.

reames accepted this revision.Jun 9 2022, 5:21 PM

LGTM w/minor required change.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1417

TSFlags appears dead, remove it before commit.

This revision is now accepted and ready to land.Jun 9 2022, 5:21 PM

Remove useless definition of TSFlags.

fakepaper56 marked an inline comment as done.Jun 9 2022, 7:14 PM
fakepaper56 added inline comments.
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
9

Done.

1417

Done.

This revision was landed with ongoing or failed builds.Jun 9 2022, 10:57 PM
This revision was automatically updated to reflect the committed changes.
fakepaper56 marked an inline comment as done.
kosarev added a subscriber: kosarev.EditedJun 10 2022, 3:28 AM

This seems to break some builds. (EDIT: Ah, see it addressed in D127477.)

$ git checkout f68cad90870590b7b1854828f255070301873347
$ cmake \
    -DBUILD_SHARED_LIBS=ON \
    -DCMAKE_BUILD_TYPE=RELEASE \
    -DCMAKE_INSTALL_PREFIX=./install \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_PROJECTS="clang" \
    -DLLVM_USE_LINKER=gold \
    -GNinja \
    ../../llvm
$ ninja
FAILED: lib/libLLVMRISCVDesc.so.15git 
: && /usr/bin/c++ -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,-z,defs -Wl,-z,nodelete -fuse-ld=gold   -Wl,--gc-sections -shared -Wl,-soname,libLLVMRISCVDesc.so.15git -o lib/libLLVMRISCVDesc.so.15git lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVAsmBackend.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVBaseInfo.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVELFObjectWriter.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVInstPrinter.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCAsmInfo.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCCodeEmitter.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCExpr.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCObjectFileInfo.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCTargetDesc.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMatInt.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVTargetStreamer.cpp.o lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVELFStreamer.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMRISCVInfo.so.15git  lib/libLLVMMC.so.15git  lib/libLLVMSupport.so.15git  -Wl,-rpath-link,/home/kosarev/labs/llvm-project/build/debug+asserts/lib && :
lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVBaseInfo.cpp.o:RISCVBaseInfo.cpp:function llvm::isFaultFirstLoad(llvm::MachineInstr const&): error: undefined reference to 'llvm::MachineInstr::getNumExplicitDefs() const'
lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVBaseInfo.cpp.o:RISCVBaseInfo.cpp:function llvm::isFaultFirstLoad(llvm::MachineInstr const&): error: undefined reference to 'llvm::MachineInstr::findRegisterDefOperandIdx(llvm::Register, bool, bool, llvm::TargetRegisterInfo const*) const'
collect2: error: ld returned 1 exit status
[18/1384] Building CXX object tools/clang/utils/TableGen/CMakeFiles/clang-tblgen.dir/ClangAttrEmitter.cpp.o
ninja: build stopped: subcommand failed.

I am sorry for creating the trouble. Should I revert the patch first?

I am sorry for creating the trouble. Should I revert the patch first?

Might help if @sunshaoce is not going to land D127477 anytime soon.

I've landed it.

llvm/test/CodeGen/RISCV/rvv/vleff-vlseg2ff-output.ll