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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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
2. Further reduce the test case
3. Add a test case for a byte multiple store and a non-multiple load
4. Add a test case for load/load forwarding (same problem exists for the def case there as well)

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

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.


https://reviews.llvm.org/D25849