This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support __builtin_nontemporal_load/store by MachineMemOperand
ClosedPublic

Authored by BeMg on Feb 5 2023, 7:39 PM.

Details

Summary

In this commit, we support nontemporal feature by checking the MachineMemOperand from
load/store MI.

There are two conditions that need to be met.

  1. The Load/Store and NTLH is immediate subsequent.
  2. Maintain the correct code size for relaxation.

For condition 1, it creates the RISCVInsertNTLHInstsPass and executes it in addPreEmitPass2.
This MachineFunction Pass does the similar thing as RISCVExpandPseudoPass
but it only expands the instruction with non-temporal flag (check through MachineMemOperand)

For condition 2, it checks the all instructions with non-temporal flag
(also through MachineMemOperand) and calculate in getInstSizeInBytes.

This way could support both scalar type and vector type (fixed-length).

Diff Detail

Event Timeline

BeMg created this revision.Feb 5 2023, 7:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2023, 7:39 PM
BeMg requested review of this revision.Feb 5 2023, 7:39 PM
BeMg edited the summary of this revision. (Show Details)Feb 5 2023, 7:40 PM

The way other targets appear to be approach this is to modify ISEL. Can you explain why you believe the late hint insertion pass is the right approach here? The alternative would be to have a family of non-temporal pseudos which expanded into a hint + a memory op. I don't have a strong opinion, just curious for the justification.

llvm/test/CodeGen/RISCV/nontemporal-c.ll
5 ↗(On Diff #494988)

These need updated to use modern "ptr" syntax.

llvm/test/CodeGen/RISCV/nontemporal-vector.ll
42 ↗(On Diff #494988)

Having scalar tests in a vector file is a bit odd...

llvm/test/CodeGen/RISCV/nontemporal.ll
466

I couple of general issues with your tests:

  • It's not really clear what the separation between the test files is. You appear to repeat tests multiple times. Did you maybe want to use multiple check lines in a single file?
  • You don't appear to have any test coverage of scalable vectors.
  • You need to update syntax to use modern "ptr".
  • You mix volatile and non-volatile without discussion on why (that I saw on a quick skim). In general, you should not be using volatile you're actually testing volatile. (i.e. don't use it just to "stabilize a test")
craig.topper added a comment.EditedFeb 24 2023, 11:38 AM

The way other targets appear to be approach this is to modify ISEL. Can you explain why you believe the late hint insertion pass is the right approach here? The alternative would be to have a family of non-temporal pseudos which expanded into a hint + a memory op. I don't have a strong opinion, just curious for the justification.

I think this was to reduce the number of vector pseudos required. There 2088 vector load pseudos today if you consider unit stride, strided, indexed(with different eews for index). Then there are segment versions of those with different seqment sizes. Then there are masked, unmasked, and tail undisturbed versions. For full generality we would need to apply non-temporal hint to all of them. Though __builtin_nontemporal_load doesn't need all of those variations we could have target specific intrinsics that do.

BeMg updated this revision to Diff 504566.Mar 13 2023, 2:43 AM
BeMg edited the summary of this revision. (Show Details)

Reorganizing the test case

  1. Use ptr instead of type*
  2. Remove volatile
  3. Merge scalar/C-ext/fixed-length into one testcase file
  4. Add scalable vector testcase (put in nontemporal-scalable.ll because the scalable type can't be compiled without the V extension)
craig.topper added inline comments.Mar 13 2023, 4:41 PM
llvm/lib/Target/RISCV/RISCVInsertNTLHInsts.cpp
57

Move the declaration and assignment of Changed to line 64, right before the loop.

71

This should be hasStdExtCOrZca()

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

COrZca

llvm/lib/Target/RISCV/RISCVInstrInfoZihintntl.td
1

This line exceeds 80 characters

14

Doesn't the Pseudo class already imply isCodeGenOnly = 1?

BeMg updated this revision to Diff 505031.Mar 14 2023, 4:38 AM
  1. Move Changed
  2. hasStdExtC -> hasStdExtCOrZca
  3. Update format
  4. Remove isCodeGenOnly
BeMg updated this revision to Diff 508878.Mar 27 2023, 8:28 PM

rebase and ping

craig.topper added inline comments.Mar 29 2023, 12:12 PM
llvm/lib/Target/RISCV/RISCVInsertNTLHInsts.cpp
58
const auto &ST = MF.getSubtarget<RISCVSubtarget>();
TII = ST.getInstrInfo();
BeMg updated this revision to Diff 509896.Mar 30 2023, 9:25 PM

Update TII

This revision is now accepted and ready to land.Mar 30 2023, 9:35 PM