This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Set CostPerUse to 1 iff RVC is enabled
ClosedPublic

Authored by pcwang-thead on Jan 19 2022, 8:34 PM.

Details

Summary

After D86836, we can define multiple cost values for
different cost models. So here we set CostPerUse to
1 iff RVC is enabled to avoid potential impact on RA.

Diff Detail

Event Timeline

pcwang-thead requested review of this revision.Jan 19 2022, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 8:34 PM
craig.topper added inline comments.Jan 19 2022, 8:48 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
353

Stylistically I'd prefer this as MF.getSubtarget<RISCVSubtarget>().hasStdExtC() ? 1 : 0

That makes it a little more obvious it is returning an index and not boolean.

llvm/test/CodeGen/RISCV/rvv/fixed-vector-strided-load-store.ll
1

Drop this line. the test is generated by both update_llc_test_checks.py and update_test_checks.py. Having the comment at the top prevents the opposite script from being able to run on the file.

Address comments.

This scheme seems rather inflexible. We can get away with it currently since the default behaviour of zero-filling CostPerUse elements out to the maximum list size works in every case, but I could see there being issues in future where you end up needing to be extremely verbose to avoid such behaviour.

This seems reasonable to me. I think @jrtc27 's is directed at the general cost per use list mechanism and not specifically about the functional change of this patch. @jrtc27, please correct me if I'm wrong.

This scheme seems rather inflexible. We can get away with it currently since the default behaviour of zero-filling CostPerUse elements out to the maximum list size works in every case, but I could see there being issues in future where you end up needing to be extremely verbose to avoid such behaviour.

Correct me if I am wrong, we shall define CostPerUse=[0, 0] explicitly for RVC registers?

I have one question: Should we set CostPerUse for floating point registers? If so, we may need to unroll the register definitions for F&D manually.

Set CostPerUse for floating point registers.

This scheme seems rather inflexible. We can get away with it currently since the default behaviour of zero-filling CostPerUse elements out to the maximum list size works in every case, but I could see there being issues in future where you end up needing to be extremely verbose to avoid such behaviour.

Correct me if I am wrong, we shall define CostPerUse=[0, 0] explicitly for RVC registers?

If I understand correctly, that would be enough to address @jrtc27's comment, no?

Overall LGTM. Maybe just clarify and address that previous point?

This scheme seems rather inflexible. We can get away with it currently since the default behaviour of zero-filling CostPerUse elements out to the maximum list size works in every case, but I could see there being issues in future where you end up needing to be extremely verbose to avoid such behaviour.

Correct me if I am wrong, we shall define CostPerUse=[0, 0] explicitly for RVC registers?

If I understand correctly, that would be enough to address @jrtc27's comment, no?

Overall LGTM. Maybe just clarify and address that previous point?

No, I would rather not do that. My concern is that if we ever want more than one dimension, much like HwMode, we will have to expand out the 2^N possibilities for every register, with most entries being duplicates. That would only be solved if you could, say, choose the index based on what register is being asked about, not just the MachineFunction, as then you could have different meanings for CostPerUse indices between different sets of registers.

craig.topper added inline comments.Jan 20 2022, 8:33 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
181

We discussed this at the RISC-V sync up this morning. I don't believe this should be part of this patch. Relaxing the restriction on non-RVC as you did in your original revision is non-controversial. Applying a new restriction on floating point with RVC may require performance measurements to be done.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
181

Thanks. I will revert this patch to the original version and create another one for floating point registers after running SPEC FP (if it's profitable).

  • Rebase.
  • Revert to the original version.
pcwang-thead marked 3 inline comments as done.Jan 20 2022, 9:38 PM
craig.topper accepted this revision.Jan 20 2022, 9:38 PM

LGTM

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
181

Code size numbers would be good too.

This revision is now accepted and ready to land.Jan 20 2022, 9:38 PM
This revision was automatically updated to reflect the committed changes.