- User Since
- Feb 10 2016, 9:51 AM (191 w, 6 d)
Update iterator var names.
Nit: rename optional argument.
I'd like to check this in, in an effort to clear my patch queue. Please let me know if you have any comments.
Mon, Oct 14
Thu, Oct 10
Thank you for the review!
Wed, Oct 9
Tue, Oct 8
Mon, Oct 7
Maybe elaborate in the patch description what determine when and how we will unroll loops. means?
"The default before and after this patch is for LoopUnroll to be enabled, and for it to use a cost model to determine whether to unroll the loop (OnlyWhenForced = false). Before this patch, disabling loop unroll would not run the LoopUnroll pass. After this patch, the LoopUnroll pass is being run, but it restricts unrolling to only the loops marked by a pragma (OnlyWhenForced = true).
In addition, this patch disables the UnrollAndJam pass when disabling unrolling."
Thu, Oct 3
Wed, Oct 2
Thank you for this!
Tue, Oct 1
I'm seeing the attached test pass at ToT now.
I may have resolved it in rL373380, which was failing to remove trivial Phis when inserting a backedge block.
Please let me know if you are still seeing issues!
Mon, Sep 30
This already landed in D62981 .
Wed, Sep 25
Tue, Sep 24
Mon, Sep 23
Fri, Sep 20
Thanks for the cleanup! A couple of small comments inline.
Thu, Sep 19
Tue, Sep 17
Gentle ping on this.
Mon, Sep 16
Perhaps worth mentioning that the walker also does phi translation when walking defs upward.
MSSA should have already done this verification. The walker will skip over stores that do not clobber the load or other MemoryAccess-es that do not interfere with the load.
So if there is a Store with the same pointer and different offset, the aliasing result for the two should be that they don't alias, and the walker will continue until it finds an access that does clobber the load.
Would something like this work for your purpose?
This is the part I do not understand. It looks like the LoadInst is considered "free" and not marked towards the cost of the unroll if there is a store with the same base pointer + offset.
Is that always the case? If there is an interfering MemoryAccess between the two (the walker's answer), then the LoadInst is not free.
I'm not that familiar with the code here, so feel free to correct me if I misunderstood.
Here are a couple of answers/discussion points:
Sep 13 2019
Make the update in MemorySSA.
Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled?
Except for a few cases, we rely on getModRefInfo to add MemoryAccesses to Instructions, so, yes, without basicaa, we're creating accesses where we shouldn't.
Would it make sense to fix that, instead of adding checks which always fail normally?
Yes, that would make a lot of sense! I was thinking of adding this as as alternative in the patch description so I'm very happy you suggested it.
Sep 12 2019
lgtm with a nit: clang-format (some lines seem longer than 80)
Thank you for the suggestion and review.
Sep 11 2019