This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Remove legacy TA/TU pseudo distinction for binary instructions
ClosedPublic

Authored by reames on Jun 30 2023, 11:33 AM.

Details

Summary

This change continues with the line of work discussed in https://discourse.llvm.org/t/riscv-transition-in-vector-pseudo-structure-policy-variants/71295.

This change handles most of the binary pseudos. I excluded pseudos which _TIED variants, and those that produce mask results. Both a bit different in functionality, and deserve their own change and review. As with previous changes in the series, we replace the existing TA and TU forms with a single unified pseudo with a passthru (which may be implicit_def) and a policy operand.

As before, we see codegen changes (some improvements and some regressions) due to scheduling differences caused by the extra implicit_def instructions.

Diff Detail

Event Timeline

reames created this revision.Jun 30 2023, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 11:33 AM
reames requested review of this revision.Jun 30 2023, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 11:33 AM
reames updated this revision to Diff 537528.Jul 5 2023, 3:39 PM
reames edited the summary of this revision. (Show Details)

Manually updated MIR tests

reames updated this revision to Diff 537554.Jul 5 2023, 5:44 PM

Add in RoundingMode variants.

craig.topper added inline comments.Jul 7 2023, 4:14 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

What happens if we don't use undef here?

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
729

Add a FIXME here so we can find it later. Unless you're planning to fix it.

llvm/test/CodeGen/RISCV/rvv/subregister-undef-early-clobber.mir
37–38

Should this be IMPLICIT_DEF instead? I see some other IMPLICIT_DEFs and no undefs in the existing code.

llvm/test/CodeGen/RISCV/rvv/vxrm.mir
1

Is this test no longer generated by the script?

reames added inline comments.Jul 10 2023, 7:31 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

If I remember correctly, you get a verifier error due to a use before definition.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
729

I'm not, so will do.

llvm/test/CodeGen/RISCV/rvv/subregister-undef-early-clobber.mir
37–38

Looks like it yeah. I got confused because this is post InsertVSETVLI. Seeing implicit vtype and implicit vl almost always means your post regalloc, and I apparently didn't look close enough. Update pending.

llvm/test/CodeGen/RISCV/rvv/vxrm.mir
1

From what I could tell, it wasn't before. It just had the header. Or at least, auto-update didn't appear to work in any meaningful way.

reames updated this revision to Diff 538653.Jul 10 2023, 8:07 AM

Address review comments

craig.topper added inline comments.Jul 10 2023, 11:38 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

Shouldn't we be using the register from Operand 1 not Operand 0?

reames added inline comments.Jul 11 2023, 8:03 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

Not unless I'm missing something?

We're forming the non-tied form of the instruction here. This used to be the unsuffixed (TA) version, and thus the merge was implicitly undef. With the new representation, we have the policy and merge operands. We have to explicitly form the undef merge operand.

Since the operand is undef, it can be any register whose class matches the def, but it conceptually makes the most sense to me to have this be the destination register given that's what it's tied to. I believe that using 0 or 1 here is NFC, provided it's flagged undef. If you have a strong preference here, I'm happy to change.

reames added inline comments.Jul 11 2023, 8:12 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

Actually, using operand 1 isn't allowed by the verifier. It's checked in MachineVerifier.cpp like so.

unsigned DefIdx;
if (MF->getProperties().hasProperty(
        MachineFunctionProperties::Property::TiedOpsRewritten) &&
    MO->isUse() && MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
    Reg != MI->getOperand(DefIdx).getReg())
  report("Two-address instruction operands must be identical", MO, MONum);
craig.topper accepted this revision.Jul 11 2023, 9:39 AM

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
2543

Ok I wasn't quite sure what side of the transition out of SSA we're on and what the Two-Address instruction pass will do after this call.

I think based on what you've said, operand 0 with undef is correct. Thanks.

This revision is now accepted and ready to land.Jul 11 2023, 9:39 AM
This revision was landed with ongoing or failed builds.Jul 11 2023, 10:21 AM
This revision was automatically updated to reflect the committed changes.