[all-commits] [llvm/llvm-project] abda8c: llvm-mca: Disentangle `MemoryGroup` from `LSUnitBa...

[André Rösti via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: abda8ce2ee2ad35af7f069fab851adaa4646d0ef
      https://github.com/llvm/llvm-project/commit/abda8ce2ee2ad35af7f069fab851adaa4646d0ef
  Author: André Rösti <an.roesti at gmail.com>
  Date:   2024-11-18 (Mon, 18 Nov 2024)

  Changed paths:
    M llvm/include/llvm/MCA/HardwareUnits/LSUnit.h
    M llvm/lib/MCA/HardwareUnits/LSUnit.cpp
    M llvm/lib/MCA/HardwareUnits/Scheduler.cpp

  Log Message:
  -----------
  llvm-mca: Disentangle `MemoryGroup` from `LSUnitBase` (#114159)

In MCA, the load/store unit is modeled through a `LSUnitBase` class.
Judging from the name `LSUnitBase`, I believe there is an intent to
allow for different specialized load/store unit implementations.
(However, currently there is only one implementation used in-tree,
`LSUnit`.)

PR #101534 fixed one instance where the specialized `LSUnit` was
hard-coded, opening the door for other subclasses to be used, but what
subclasses can do is, in my opinion, still overly limited due to a
reliance on the `MemoryGroup` class, e.g.
[here](https://github.com/llvm/llvm-project/blob/8b55162e195783dd27e1c69fb4d97971ef76725b/llvm/lib/MCA/HardwareUnits/Scheduler.cpp#L88).

The `MemoryGroup` class is currently used in the default `LSUnit`
implementation to model data dependencies/hazards in the pipeline.
`MemoryGroups` form a graph of memory dependencies that inform the
scheduler when load/store instructions can be executed relative to each
other.

In my eyes, this is an implementation detail. Other `LSUnit`s may want
to keep track of data dependencies in different ways. As a concrete
example, a downstream use I am working on<sup>[1]</sup> uses a custom
load/store unit that makes use of available aliasing information. I
haven't been able to shoehorn our additional aliasing information into
the existing `MemoryGroup` abstraction. I think there is no need to
force subclasses to use `MemoryGroup`s; users of `LSUnitBase` are only
concerned with when, and for how long, a load/store instruction
executes.

This PR makes changes to instead leave it up to the subclasses how to
model such dependencies, and only prescribes an abstract interface in
`LSUnitBase`. It also moves data members and methods that are not
necessary to provide an abstract interface from `LSUnitBase` to the
`LSUnit` subclass. I decided to make the `MemoryGroup` a protected
subclass of `LSUnit`; that way, specializations may inherit from
`LSUnit` and still make use of `MemoryGroup`s if they wish to do so
(e.g. if they want to only overwrite the `dispatch` method).

**Drawbacks / Considerations**

My reason for suggesting this PR is an out-of-tree use. As such, these
changes don't introduce any new functionality for in-tree LLVM uses.
However, in my opinion, these changes improve code clarity and prescribe
a clear interface, which would be the main benefit for the LLVM
community.

A drawback of the more abstract interface is that virtual dispatching is
used in more places. However, note that virtual dispatch is already
currently used in some critical parts of the `LSUnitBase`, e.g. the
`isAvailable` and `dispatch` methods. As a quick check to ensure these
changes don't significantly negatively impact performance, I also ran
`time llvm-mca -mtriple=x86_64-unknown-unknown -mcpu=btver2
-iterations=3000 llvm/test/tools/llvm-mca/X86/BtVer2/dot-product.s`
before and after the changes; there was no observable difference in
runtimes (`0.292 s` total before, `0.286 s` total after changes).

<sup>[1]: MCAD started by @mshockwave and @chinmaydd.</sup>



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications