[PATCH] D25849: [GVN] Prevent load coercion with irregular vector types
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 31 09:47:32 PST 2017
davide added a comment.
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)
> 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.
> "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?
More information about the llvm-commits