[libc-commits] [PATCH] D111621: [libc] automemcpy - function generator

[Clement Courbet via Phabricator via libc-commits]

courbet added inline comments.


================
Comment at: libc/benchmarks/automemcpy/include/automemcpy/RandomFunctionGenerator.h:34
+  // Returns an expression where `Variable` is forced to be one of the `Values`.
+  z3::expr disjunction(z3::expr &Variable, ArrayRef<int> Values) const;
+  void addBoundsAndAnchors(z3::expr &Begin, z3::expr &End);
----------------
This is not really a "disjunction", more like a disjuction of equality clauses. What about simply calling this `addInSetConstraint` or something like this ?


================
Comment at: libc/benchmarks/automemcpy/include/automemcpy/RandomFunctionGenerator.h:35
+  z3::expr disjunction(z3::expr &Variable, ArrayRef<int> Values) const;
+  void addBoundsAndAnchors(z3::expr &Begin, z3::expr &End);
+  void addLoopConstraints(const z3::expr &LoopBegin, const z3::expr &LoopEnd,
----------------
doc


================
Comment at: libc/benchmarks/automemcpy/include/automemcpy/RandomFunctionGenerator.h:36
+  void addBoundsAndAnchors(z3::expr &Begin, z3::expr &End);
+  void addLoopConstraints(const z3::expr &LoopBegin, const z3::expr &LoopEnd,
+                          z3::expr &LoopBlockSize, int LoopMinIter);
----------------
doc


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:20
+
+// Exploration parameters
+static constexpr int kLoopMinIter = 3;
----------------
Can you please document these ?


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:55
+                               }));
+  // But disallow Bcmp for now.
+  Solver.add(Type != (int)FunctionType::BCMP);
----------------
Why not simply comment out bcmp int the type array above ?


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:58
+
+  // Span bounds are bounded.
+  addBoundsAndAnchors(ContiguousBegin, ContiguousEnd);
----------------
This is not super clear. Maybe:

```
// Add constraints for span bounds.
```

The reader can then refer to `addBoundsAndAnchors` to understand what the specific constraints are.


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:64
+  addBoundsAndAnchors(AcceleratorBegin, AcceleratorEnd);
+  // Fix endpoints.
+  Solver.add(ContiguousBegin == 0);
----------------
```
// Fix endpoints: The minimum size that we want to copy is 0, and we always start with the `Contiguous` strategy. The max size is `kMaxSize`, and we always end with the `Accelerator` strategy, as it's typically more efficient for large sizes.
```


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:68
+  // Spans are ordered relative to each others.
+  Solver.add(ContiguousEnd == OverlapBegin);
+  Solver.add(OverlapEnd == LoopBegin);
----------------
```
// We always consider strategies in this order: Contiguous <= Overlap <= Loop <= AlignedLoop <= Accelerator
```


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:73
+  // Contiguous
+  Solver.add(ContiguousEnd <= 5); // up to Size == 4
+  // Overlap
----------------
Why ? Can you make this a kConstant ?


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:75
+  // Overlap
+  Solver.add(OverlapEnd <= 257); // up to Size == 256
+  Solver.add(OverlapBegin == OverlapEnd || OverlapBegin >= 2);
----------------
ditto


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:76
+  Solver.add(OverlapEnd <= 257); // up to Size == 256
+  Solver.add(OverlapBegin == OverlapEnd || OverlapBegin >= 2);
+  // Loop
----------------
This needs a comment.


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:82
+                     kAlignedLoopMinIter);
+  Solver.add(disjunction(AlignedAlignment, {16, 32, 64}));
+  Solver.add(AlignedLoopBegin == AlignedLoopEnd || AlignedLoopBegin >= 64);
----------------
please get rid of the magic numbers and hoist them to constants at the top.


================
Comment at: libc/benchmarks/automemcpy/lib/RandomFunctionGenerator.cpp:99
+                                        (int)ElementTypeClass::BUILTIN}));
+  // But we only keep native for now.
+  Solver.add(ElementClass == (int)ElementTypeClass::NATIVE);
----------------
ditto: no need to add then remove, let's have fewer constraints instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111621