- User Since
- Feb 10 2016, 9:51 AM (166 w, 1 d)
Address comments. Thank you for the review!
It's not as much the ew-ness that bothers me.
It's that, if MemorySSA is to be preserved and widely used in the loop pass, then this infrastructure change is needed anyway.
Rebase on top of patches updating LCSSA and LoopSimplify.
Simplify patch to preserve MemorySSA in the old pass manager only when the pass is already available (remove *require*).
New pass manager does not preserve it.
Simplify patch to only rpeserve and not require.
Rebase after change that parses vectorize flags.
Wed, Apr 17
Fri, Apr 12
Thank you for the review!
Tue, Apr 9
Mon, Apr 8
Attempting to merge defaults to a single cl::opt.
Please note the comments on defaults in lib/Transforms/Vectorize/LoopVectorize.cpp. Feedback there very welcome.
Fri, Apr 5
Yes, that's almost certain the case.
Thu, Apr 4
Here are the times I got for compiling the aggregated bitcode files for clang and a few others. Before is ToT, After is with -forget-scev-loop-unroll=true
File Before (s) After (s)
clang-9.bc 7267.91 6639.14
llvm-as.bc 194.12 194.12
llvm-dis.bc 62.50 62.50
opt.bc 1855.85 1857.53
Tue, Apr 2
I'm looking to see if a reduced case I have can be open-sourced in a PR and have this patch point to it.
Following the introduction of BatchAA (https://reviews.llvm.org/D59315), this patch is certainly *not* an option, because it would block using BatchAA in the AliasSetTracker (https://reviews.llvm.org/D59438). Using BatchAA is not correct if there are subsequent alias calls after doing CFG changes. And calling addPointer with anything but true as the next to last argument may make an alias call.
Stale, no longer applies.
Mon, Apr 1
LGTM, modulo one nit. Thank you!
Thank you for this!
Thank you for the cleanup!
A couple of quick comments.
Would you mind splitting the cleanups and bug fix (which will be straightforward to approve) and rebase this on top of that?
Fri, Mar 29
Thank you for the review!
Thank you for the fix!
Address comments and asking for clarifications.
Thu, Mar 28
LGTM. Thank you!
Fri, Mar 22
Thu, Mar 21
Rewrite some of the documentation.
Getting back to this, do you have a suggestion for making progress on fixing this mis-compile?
Is the alternative in D58746 an option?
Creating the QueryInfo on the stack and removing the clear method.
Wed, Mar 20
Thank you for the comments!
Quick answer before addressing the rest of the comments: the missing clear() calls were intentional, since they're redundant. The calls that don't call clear() are just wrappers over the methods that do call clear(). These wrappers are over a single call so there wouldn't be any benefit from not clearing inside the wrapped method, even if we could.
I'm not sure if it's preferable to add the redundant calls for consistency or safety if things change in the future, or add comments explaining why they're not needed today.
Rebase and ping for comments.