[PATCH] D25849: [GVN] Prevent load coercion with irregular vector types

Wed Feb 1 15:41:56 PST 2017

davide added a comment.

In https://reviews.llvm.org/D25849#664119, @reames wrote:

> In https://reviews.llvm.org/D25849#661788, @davide wrote:
>
> > In https://reviews.llvm.org/D25849#661773, @reames wrote:
> >
> > > Ok, I've had a chance to review the code more closely.  The alternate approach I'd suggested does look to be a lot more work.  More generally, it looks like *all* of the GVN code gives up on non-byte multiple values, this is simply fixing one particular path through that was missing a check.
> > >
> > > For the actual change at hand, I am okay with this going in, provided the following is done:
> > >
> > > 1. Add the check for load size
> >
> >
> > Do we really need this?
> >
> > > 2. Further reduce the test case
> >
> > Ditto (it's as 7 lines testcase).
> >
> > > 3. Add a test case for a byte multiple store and a non-multiple load
> >
> > I don't see this asked in the original review.
> >
> > > 4. Add a test case for load/load forwarding (same problem exists for the def case there as well)
> >
> > Ditto.
>
>
> I'm willing to consider a patch with a subset of these, but I'm not going to discuss this abstractly.  If you want me to consider a patch, please update the review.
>
> >> Now, let me turn to the meta discussion.
> >> 
> >> Danny, I really don't think this patch *does* meet our standards for the following reasons:
> >> 
> >> 1. Comments on the review were ignored.  (Checking load size)
> > 
> > Not quite. See my previous reply:
> >  "are you OK with me updating the patch (i.e. adding the check) and going forward?"
>
> You are correct about the comment, but the patch has never been updated.  Given that, the discussion about whether it can be committed is premature.
>
> >> 2. When asked about alternate approaches (which is pretty much the purpose of review), author did not do so.  To be clear, I'm perfectly fine if the alternate approach is *rejected after consideration*, but that *after consideration* part is important.
> > 
> > And I pointed out it's not worth my time (but you can disagree, and fix that yourself) fixing load coercion for vectors of irregular types. Crashes are bad, IMHO, so I decided to take a look. Easy peasy.
>
> Asking for an alternate approach to be considered is normal part of review.
>
> > Please note that the current patch is already an improvement on the current state of things, as in, hey, at least we don't crash anymore and we don't provide a wrong result.
>
> We have a long history of not accepting low quality patches, even for crashes.

Let's agree to disagree. You still haven't pointed out anything wrong with the patch itself. Danny already provided an explanation why this is an OK (even if not perfect) fix to accept. That said, low quality code IMHO includes (but not limited to): untested patches (which is not the case here), patch with obvious mistake (again, the check seems correct, here), etc.. 
With that in mind, I tend to agree that the patch can be extended to catch other cases. I also think that these can be considered separately. As I pretty much have zero interest in discussing this further with you (and I consider it a very poor use of my time), I'll just ignore and go on with my life/work ;)

>>> 3. There is no discussion in the review of *what issue is being fixed*.  Something simple along the lines of "GVN doesn't handle non byte multiple sizes.  We correctly protect the clobber paths, but missed a case on the def path." would have gone a long way to simplifying the review here.  Also, there was no discussion in the *review* that this was a regression.
>> 
>> ehm, this is fixing a crash.
> 
> That is not mentioned in the review description.  It is only in the linked bug.  Explaining *how/why* the crash happened is expected and required.

The review description has a link containing the crash. Saying that I didn't copy/pasted that in the review is a little weak as argument.

>>> 4. Patch does not consider alternate ways of triggering essentially the same bug.  (i.e. the load case)  Test cases were extremely minimal and didn't provide good coverage of the previous broken code.  It is not a strict *requirement* that adjacent bugs be fixed, but it is certainly *good practice* to look for them when the first problem report comes in.
>> 
>> This patch is fixing a bug. Related bugs can be considered (if I'd really choose to do so) as follow-ups. We do that in LLVM all the time.
>> 
>>> The "trend" I was referencing was specifically in reference to a previous GVN patch (hoisting non-locals to entry block) which the author chose to revert rather than discuss when asked about it.
>>> 
>>> "I'm busy with other things" is not a reason to relax our usual review standards.  There are days I wish it was - it would certainly make my life easier - but from the perspective of the project accepting bad code because the author is busy is a deeply dangerous idea.
>> 
>> Not my words =) If I was you I would be more careful with quotes =) Also, what is the bad code?
> 
> My apologies.  I did quote where I shouldn't have.  That was the way I read your response, but not words you actually said.
> 
> Merging other comments so I can reply in one...
> 
>> Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)
> 
> You're correct that in an ideal world I should have replied sooner.  I will point out that I volunteer my time for things like this and do not promise to ever respond to a given review thread unless it looks like a good use of my time.
> 
>> And last I checked, reverting dubious patches/patches where objections are raised is not an uncommon practice in LLVM (in fact, IIRC, we encourage early revert in case there are problems, e.g. bots broken, issues in the design). I also would like to point out it's an isolated case.
> 
> Ok, you've brought up this point a couple of times.  I assume you're referring to the refactoring change from early last year which exposed this crash?  If you'd proposed a revert of patch which a clear test case, I most likely would not have objected.  You did not do this.  You posted a patch without clear context and then failed to respond to review comments with a revised patch.
> 
>> And I actually proposed to work an alternative, that is NewGVN (which actually was committed shortly after, and it's going very strong, thanks to the burst of recent commits from Danny).
> 
> As I said before, working on an alternative solution is not a reason to accept low quality patches for the infrastructure currently in use.

https://reviews.llvm.org/D25849