[PATCH] D143323: [ConstraintElim] Add reproducer remarks.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 09:13:56 PST 2023
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
More information about the llvm-commits
mailing list