[PATCH] D42032: [LLVM][PASSES][InstCombine] Fix (a + a + ...) / a cases

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D42032#982440, @AntonBikineev wrote:

> In https://reviews.llvm.org/D42032#981882, @spatel wrote:
>
> > This looks right, but see inline comments for some small improvements. Do you have commit access?
>
>
> No, unfortunately.


No problem; I can commit on your behalf when the patch is complete. I can check in the tests first if you agree with my explanation on that.

>> No need to initialize this; we don't touch 'Y' if the matches fail.
> 
> In this particular case it's correct - we eliminate the extra store, but on the other hand IMHO it promotes a good code style by not accidentally reading uninitialized data.

I think the argument goes the other way - we want to see a compiler warning if we accidentally read the variable uninitialized, so we purposely leave it uninitialized to allow that possibility.

>> Probably best not to speculatively create this constant. I think you can move this into the 'Create' lines below without violating the 80-column limit. :)
> 
> But then we would either need to duplicate this line in both statements or use an extra check... I don't know if it's better :)

It's correct that you'll duplicate the line, but I think that's better than potentially executing code that won't be used.


https://reviews.llvm.org/D42032