This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a cross basic block VXRM write insertion pass.
Changes PlannedPublic

Authored by craig.topper on Aug 25 2023, 4:40 PM.

Details

Summary

This is based on D122709 without the loop hoisting support. It has
been updated to be more consistent with the dataflow in handling in
the current vsetvli insertion pass.

I've separated VXRM handling from FRM handling since FRM requires
restoring and VXRM doesn't. We may even consider moving VXRM insertion
after register allocation to give more scheduler freedom.

A future patch will support hoisting out of loops.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 25 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 4:40 PM
craig.topper requested review of this revision.Aug 25 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 4:40 PM

The heuristic overall makes sense to me. Maybe add test coverage for nested loops too?

llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp
22

folowing -> following

if one of the following conditions meet?

57

Can we have a static singleton of VXRMInfo whose State is Unknown?

70

Add assertion?

assert(0 <= Imm && Imm <= 3 && "vxrm value range is [0, 3]");
127

Is this extra line that was copied from elsewhere?

158

Describing this as "intersect" of all predecessors might be more descriptive.

208

const

212

const

214

const

llvm/test/CodeGen/RISCV/rvv/vxrm-insert.ll
145

Nit: Maybe more comprehensive test function names.

craig.topper marked an inline comment as done.Aug 28 2023, 11:35 AM
craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp
127

I copied part of this from vsetvli. Not sure if I there's a mistake there too or I just didn't copy enough.

212

I don't see a lot of value in putting const on a short lived local variable

214

I don't see a lot of value in putting const on a short lived local variable

craig.topper added inline comments.Aug 28 2023, 11:44 AM
llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp
57

I think it'd like to keep it this way. It's the same in VSETVLIInfo. I'd need to add a new constructor in order to be to create the singleton in the unknown state.

Address comments

craig.topper planned changes to this revision.Aug 28 2023, 7:45 PM

Can you precommit the test so that the deltas are visible?

General structure makes sense, minor style comments only.

llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp
12

This comment confuses me, and doesn't seem to correspond to the code. Any chance this is stale?

59

Minor: isInitialized

79

I believe this can be written as:

if (!isValid() || !Other.isValid())
  return isValid() == Other.isValid();

Same with the unknown check just below.

105

You can simplify this as:

if (*this == Other)
      return *this;
return VXMMInfo::getUnknown();

In particular, you don't need to separately check for unknown.

llvm/test/CodeGen/RISCV/rvv/vxrm-insert.ll
214

Not related to this change, but shouldn't we be hoisting these two instructions which are common on both blocks? Or is this transform running too late for that?

craig.topper added inline comments.Aug 29 2023, 10:43 PM
llvm/lib/Target/RISCV/RISCVInsertWriteVXRM.cpp
275

This is a bug, we need to add the operand even if we don't emit WriteVXRMImm