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

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

In https://reviews.llvm.org/D25849#661821, @davide 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.
> >
> > > 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?"
> >
> > > 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.
> >  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.
> >
> > > 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.
> >
> > > 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.
>
>
> Note that I also never ignored any comments, while I'm still waiting for a reply from you from Oct28th =)


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.  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).


https://reviews.llvm.org/D25849