[PATCH] D143323: [ConstraintElim] Add reproducer remarks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 5 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:778
 
-static bool checkAndReplaceCondition(CmpInst *Cmp, ConstraintInfo &Info) {
+struct ReproducerEntry {
+  CmpInst *Cond;
----------------
tschuett wrote:
> paquette wrote:
> > doxygen comments for this?
> You can, may, or should wrap the struct with an anonymous namespace. You are very careful with static.
Added comment and moved to anon namespace, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:791
+static void generateReproducer(CmpInst *Cond, Module *M,
+                               ArrayRef<ReproducerEntry> Stack,
+                               DenseMap<Value *, unsigned> &Value2Index,
----------------
paquette wrote:
> Can you document this parameter?
Documented the missing arguments, thanks!


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:803
+  bool IsSigned = CmpInst::isSigned(Cond->getPredicate());
+  auto CollectArguments = [&](CmpInst *Cond) {
+    if (!Cond || IsSigned != CmpInst::isSigned(Cond->getPredicate()))
----------------
paquette wrote:
> This lambda is long enough that I feel like it ought to just be a function (if possible)
Hmm, it would be possible but would mean passing a bunch of extra parameters that can conveniently be captured here. Happy to move it out if you think it is strictly better, but IMO having the lambda keeps it a bit simpler and limits the scope to the function here.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:846
+
+  for (unsigned i = 0; i < Args.size(); i++) {
+    Old2New[Args[i]] = F->getArg(i);
----------------
paquette wrote:
> These loops can be merged into one loop
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143323