[PATCH] D49229: [InstCombine] Fold redundant masking operations of shifted value

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio added a comment.

In https://reviews.llvm.org/D49229#1185608, @spatel wrote:

> > So all expensive operations have been eliminated, I do not see why it wouldn't fit in InstCombne. We detect a pattern and we reduce it.
>
> Because the pattern that we are matching is larger than it needs to be (as the comment in the test file clearly shows - there is no 'or' in the minimal pattern). This problem of trying to make everything fit in instcombine has been discussed several times on llvm-dev in the last ~year. Eg:
>  http://lists.llvm.org/pipermail/llvm-dev/2017-September/117151.html


I agree that it won't handle all cases. But one will need to come with a more generic thinking as to create a new pass that handles this. Something like an abstract known bits, that tells that two values hold the same bits coming from a given instruction, or some simplification by demanded bits from the same values. It is feasible, but it is not my intention to do it so now.

> Did you look at (new)-GVN to see if it fits in there?

I must confess that I did not quite understand all the work-flow of newGVN, but from what I did see, it mostly wouldn't fit. It seems to behave like InstCombine, expecting to replace the current instruction being visited. And it would require to create one value as to detect if there is a leader of that value and then reuse it. It is not that complicated, but quite awkward IMO.

> If this is in instcombine (in addition to missing the pattern when there is no 'or'), I think you have to limit the transform based on uses as Roman mentioned in an earlier comment.

Why? The `@mulUses` in the test shows the case where it does not require `%x2` to have a single user. The `shift` operation dominates the `and` being replaced.

And I agree with @samparker, our motivating example is quite simple, is the `@foo` in our tests. I just intended to made it more generic so it would act in more shift operations, not to over complicate it. I acknowledge that doing the transformation in the IR could be good, but the DAG fits much simpler. So I really see no problems in having it both places (as in https://reviews.llvm.org/D48278). Either way, it would be nice to come to a definitive solution.


https://reviews.llvm.org/D49229