[PATCH] D13606: [Introduction] Redundant load reduction with invariant intrinsics

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 15 08:49:40 PDT 2015


I'm with Philip on this one.
This patch looks like it  can be split up.
As a rule, you don't have to make everything go at once, you can do it
piece by piece.

(IE it's fine to modify one transform/analysis pass, even if nothing
but tests use it yet, and then add the use in a separate patch).

More to the point, the feedback you are getting says: "If you want
this reviewed by reviewers, you should split it up more".
So unless you can point to concrete reasons it can't be split up, you
should do that.

On Wed, Oct 14, 2015 at 3:59 PM, Philip Reames
<listmail at philipreames.com> wrote:
> reames added a comment.
>
> In http://reviews.llvm.org/D13606#264048, @lvoufo wrote:
>
>> In http://reviews.llvm.org/D13606#263966, @reames wrote:
>>
>> > This definitely needs to be split up into a series of smaller patches to
>> >  be reviewable.  I strongly suggest starting small, getting through one
>> >  cycle of review, then posting a couple more and recursing.  Reading
>> >  through the diff, I see a lot of points which will need
>> >  discussion/clarification.
>> >
>> > Also, a bit of background on what you're trying to accomplish is
>> >  required.  A link to a private google doc is not sufficient.
>> >
>> > Philip
>>
>>
>> Hi Philip -- This *is* the initial patch. I can't think of another way to break it down more effectively.
>
>
> Er, no.  This patch currently involves changes to 5 different transform passes and 3 analysis passes.  This can and must be split into smaller changes.  I'd suggest striping out everything not required for *one* of the transform passes and iterate on that.
>
>> Thanks for pointing out the inaccessibility of the design doc.
>
>>  It is not meant to be private. If one is unable to access, I believe s/he should be able to  request access, which I will promptly grant (?).
>
>>  Alternatively, I have made a copy available from my personal (and more public-capable) email account.
>
>>  Please let me know if you encounter any more issue.
>
>
> Thank you for sharing the doc publicly.  It did help to explain the problem you're trying to solve, but it didn't really give me any insight into how your trying to solve it.  Can you sketch out an architectural level summary of how you intent this to work?
>
>
> http://reviews.llvm.org/D13606
>
>
>


More information about the llvm-commits mailing list