[PATCH] D152568: [InstCombine] Transform `(binop1 (binop2 (lshift X,Amt),Mask),(lshift Y,Amt))`

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:812
+      // Pass
+    }
+    // If both BinOp1 and BinOp2 are bitwise ops and:
----------------
goldstein.w.n wrote:
> nikic wrote:
> > goldstein.w.n wrote:
> > > nikic wrote:
> > > > Does this case trigger for anything other than non-splat vectors? If not, please remove it. (Generally, don't add non-splat vector support it it requires //any// additional complexity.)
> > > Why is that? I'd argue this is very minimal complexity and think it is an issue that a significant number of our combines only work for splats. Generally we mark combines that fail because of lacking splat support not as "*_fail", but "*_todo" which seems to imply we *want* to handle them. Leaving out these 4-lines of code (that also provide an early exit before more expensive checks) seems equivelent to choosing to leave a simple addressable TODO.
> > It's not a lot of code, but it does increase the verification space of the transform non-trivially, for a transform that already has some combinatorial explosion going on.
> > 
> > Anyway, I'm okay with keeping this, but I would suggest thinking about whether non-trivial non-splat vector handling is really a good use of implementation, review and long-term maintenance time, for the optimization benefit it provides.
> > It's not a lot of code, but it does increase the verification space of the transform non-trivially, for a transform that already has some combinatorial explosion going on.
> > 
> > Anyway, I'm okay with keeping this, but I would suggest thinking about whether non-trivial non-splat vector handling is really a good use of implementation, review and long-term maintenance time, for the optimization benefit it provides.
> 
> I guess this (`add` + `lshr`) highlight a difference in philosophy between us (that has come up on other reviews). My general feeling is the more that can be covered, the better. Its a lot more painful to have to rewrite code because the compiler isn't getting your case and updating the compiler doesn't reap rewards for most until at least a year or so down the line. More cases does imply code complexity, but generally think a clean 10k LOC is clearer than 1k messy LOC i.e, as long as coding standards are kept high and the niche cases don't degrade the quality of the file, they are worth it.
> Maybe I'm trivializing some aspect of it however.
> 
My general philosophy here is that transforms should be "principled", and not based around long lists of special cases. For pragmatic reasons, I am willing to accept unprincipled transforms for cases that have been encountered in real code, but I prefer not to have unprincipled generalizations of transforms "just because we can".

For example, if you want to implement a fold for `eq`, you should always also handle `ne`, otherwise you're just handling one special case of a more general fold. If that same fold also works for unsigned predicates, it would be strongly encouraged to handle that as well. But if it's possible to generalize that fold to signed predicates, but this requires introducing special cases for known sign bit, known power of two, known bits and signed min, then ... that's not a principled extension, it's just a bag of random special cases. Adding those special cases may be fine if there is reason to believe that they will actually be useful. But we should not add those special cases just because it leaves the transform "incomplete" in a rather pedantic sense. At least that's my 2c.

Of course, the point where we go from a principled extension to a bag of special cases is subjective. The particular case that started this discussion isn't really problematic (it's really less of a special-case extension and more a precisely expressed pre-condition), and deserves less attention than this discussion ended up giving it ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152568/new/

https://reviews.llvm.org/D152568