This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Canonicalize towards vmerge w/passthrough representation
ClosedPublic

Authored by reames on Jun 7 2023, 9:23 AM.

Details

Summary

This is the first patch in a series to change how we represent tail agnostic, tail undefined, and tail undisturbed operations. In current code, we tend to use an unsuffixed pseudo for undefined (despite calling it TA most places in code), and the _TU form for both agnostic and undisturbed (via the policy operand).

The key observation behind this patch is that we can represent tail undefined via a pseudo with a passthrough operand if that operand is IMPLICIT_DEF (aka undef). We already have a few instances of this in tree - see vmv.s.x and vslide* - but we can do this more universally. Once complete, we will be able to delete roughly 1/2 of our vector pseudo classes.

This patch doesn't actually remove the legacy unsuffixed pseudo as there's still some path from intrinsic lowering which uses it. (I have not yet located it.) This also means we don't have to modify any of the lookup tables which makes the migration simpler. We can defer deleting the tables and pseudos until one final change once all the instructions have been migrated.

There are a couple of regressions in the tests. At first, these concerned me, but it turns out that all of them are differences in expansion of a single source level instruction. I think we can safely ignore this for the moment. I did explore changing the handling of IMPLICIT_DEF in ScheduleDAG, but that causes an absolutely *massive* test diff with minimal profit. I really don't think it's worth doing.

Diff Detail

Event Timeline

reames created this revision.Jun 7 2023, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:23 AM
reames requested review of this revision.Jun 7 2023, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:23 AM
reames added inline comments.Jun 7 2023, 9:28 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3455

Note - I'm removing the TA support entirely here, but given there's still some intrinsic users, we should probably restructure to add recognition of the implicit_def operand to _TU case without dropping the TA support. This will make this change reviewable (though sadly not really testable) on it's own.

llvm/test/CodeGen/RISCV/rvv/vector-splice.ll
12

Note - This change (and others like it) look concerning. I dug far enough in to this to be fairly sure this is a problem with pre-RA-sched, but haven't yet fully isolated the cause. I suspect there's some missing special casing for IMPLICIT_DEF somewhere, but I haven't figured out exactly where yet.

craig.topper added inline comments.Jun 7 2023, 10:26 AM
llvm/test/CodeGen/RISCV/rvv/vector-splice.ll
12

For the one case I looked at, it looks like we're getting different ordering out of the SelectionDAG list scheduler when we convert to MIR. Mostly that sceduler uses IROrder from the debug location. If the IR order of nodes is the same, which will occur if something was expanded into multiple SelectionDAG nodes, the list scheduler starts examining other things to break the tie.

reames updated this revision to Diff 530555.Jun 12 2023, 9:47 AM
reames retitled this revision from [WIP][RISCV] Canonicalize towards vmerge w/passthrough representation to [RISCV] Canonicalize towards vmerge w/passthrough representation.
reames edited the summary of this revision. (Show Details)

Update to preserve handling for TA pseudo version in the DAG combine.

luke accepted this revision.Jun 12 2023, 1:17 PM

LGTM

This revision is now accepted and ready to land.Jun 12 2023, 1:17 PM
frasercrmck accepted this revision.Jun 13 2023, 1:28 AM

LGTM too, makes sense and is a cleaner representation in my opinion.

craig.topper accepted this revision.Jun 13 2023, 2:02 PM

LGTM other than that typo

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3256

psuedo -> pseudo at end of the line

craig.topper added inline comments.Jun 13 2023, 2:03 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3243

Is this clang-format correctly? I would think V.getMachineOpcode() should start at the same column as V.isMachineOpcode()

This revision was landed with ongoing or failed builds.Jun 13 2023, 4:41 PM
This revision was automatically updated to reflect the committed changes.