[llvm] [LoopVectorize] Refine runtime memory check costs when there is an outer loop (PR #76034)

David Sherwood via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 19 05:10:35 PST 2024


david-arm wrote:

> Thanks for the updates!
> 
> > > Trying a rebase to see if that fixes the failing RISCV test that appears unrelated to my change
> 
> Alternatively you could use the `Update Branch` button to merge in the changes from current main, which I think is preferred to rebases, as the GH UI may lose comments after rebases.

Yeah I used to use the "Update Branch" button, but I find it makes life very difficult for reviewers (and authors) and I'd like to stop using it in future. The problem is that in Phabricator we could do stacked patches quite easily, i.e. NFC test patch followed by code change. The purpose of this is to allow people to see the impact of the code changes. In this new world we're supposed to always add a new commit every time we address comments, which completely defeats the whole purpose of splitting the test patch up from the code change. Quite often reviewers ask authors to rewrite tests or add new ones, which means that when you add a new commit you can no longer see the impact of your code change. It feels like we should either:

1. Stop splitting commits up into NFC test + code change patch, because it's not that useful anymore. Or
2. Force push the same two commits every time similar to how we did things on Phabricator. That way when we do change the tests it's still possible for the reviewer to see what effect the code change has.

To be honest either approach is unsatisfactory, but I feel option 2) gives the reviewer a more fighting chance of seeing cause and effect!

https://github.com/llvm/llvm-project/pull/76034


More information about the llvm-commits mailing list