[libc-commits] [PATCH] D111554: [libc] automemcpy README and main include file

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/benchmarks/automemcpy/FunctionDescriptor.h:1
+//===-- Pod structs to describe a memory function----------------*- C++ -*-===//
+//
----------------
courbet wrote:
> This is supposed to include the filename: https://llvm.org/docs/CodingStandards.html#file-headers
The whole libc project would need to be fixed :-/
Can we fix it separately?


================
Comment at: libc/benchmarks/automemcpy/FunctionDescriptor.h:23
+
+#define COMPARABLE_AND_HASHABLE(T, ...)                                        \
+  inline auto asTuple() const { return std::tie(__VA_ARGS__); }                \
----------------
courbet wrote:
> Do you also want to add a static_assert for PODness ?
Actually I'd only need them to be `std::trivial` but I just figured out that the `Optional` fields in `FunctionDescriptor` are stepping in the way...
The generated code is still pretty efficient https://godbolt.org/z/zdoh5vWMj


================
Comment at: libc/benchmarks/automemcpy/FunctionDescriptor.h:52
+// if(size == 1) return Handle<1>();
+struct Contiguous {
+  SizeSpan Span;
----------------
courbet wrote:
> Should the naming be `ContiguousStrategy`, `OverlapStrategy`, ... ?
> 
> I'm seeing this as describing a strategy to be applied to a given size range.
In theory yes, but in practice the type is serialized in the autogenerated C++ file and adding a trailing "Strategy" on every type would really hinder readability.
e.g. of one of the several thousand lines of serialized `NamedFunctionDescriptor`
```
    {"memcpy_0xE01C197FDF1FC6D3",{FunctionType::MEMCPY,Contiguous{{0,2}},Overlap{{2,64}},Loop{{64,128},16},AlignedLoop{Loop{{128,256},32},16,AlignArg::_1},Accelerator{{256,kMaxSize}},ElementTypeClass::NATIVE}},
```

Note: because the fields are `Optional` we have to write the type name so the compiler can distinguish between `llvm::None` construction and `T` construction, it also helps readability. Contrary to most TableGen files in LLVM, the autogenerated file will eventually be read to compare implementations.


================
Comment at: libc/benchmarks/automemcpy/FunctionDescriptor.h:80
+
+// Same as Loop but starts by aligning a buffer on Alignement.
+struct AlignedLoop {
----------------
courbet wrote:
> How ? (I mean which strategy)
I've reworded it, let me know what you think.


================
Comment at: libc/benchmarks/automemcpy/FunctionDescriptor.h:117-118
+// every details but is enough to uniquely identify the implementation. The
+// fields are ordered by priority. i.e. If several spans handle a particular
+// size, the first one handles it.
+struct FunctionDescriptor {
----------------
courbet wrote:
> Should that be an error ? The Z3 model should not be generating these given the constraints, right ?
I wrote this to allow having an individual size in the middle of an overlap:
e.g.
```
...
if(size == 24) return Op<24>();
if(size < 16) return Op<Overlap<8>>();
if(size < 32) return Op<Overlap<16>>();
```

In this example the range 8-31 is handled by an overlapping strategy expect the size 24 which could be particularly hot and worth optimizing for. It is not in the automemcpy paper but though it would be a nice addition.
I'll remove it for now since it's not implemented and probably confusing.


================
Comment at: libc/benchmarks/automemcpy/README.md:3
+
+It uses the [Z3 theorem prover](https://github.com/Z3Prover/z3) to enumerate a subset of valid memory function implementations. These implementations are then materialized as C++ code and can be [benchmarked](../) against various [size distributions](../distributions). This process helps the design of efficient implementations for a particular environnement (size distribution, processor or custom compilation options).
+
----------------
courbet wrote:
> Did you mean to give a real link here ?
It really is the parent directory.
It has to be relative path so it works in github.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111554/new/

https://reviews.llvm.org/D111554