[PATCH] D142038: [X86] canonicalizeShuffleWithBinOps - add handling of SSE vector shift nodes

Thu Jan 19 09:02:10 PST 2023

lebedev.ri added a comment.

In D142038#4065083 <https://reviews.llvm.org/D142038#4065083>, @RKSimon wrote:

> I'm sorry if I've caused you an issue here, but you did ask on D141877 <https://reviews.llvm.org/D141877> that I show you my alternative approach, and I did my best to get something to you as fast as possible, despite me explaining that I'm stretched at the moment.
>
> The level of refactoring that you employ in your version is more than is needed to add support for the SSE shifts to fix the regressions on D141778 <https://reviews.llvm.org/D141778>, and I don't agree that generalizing the code so much is necessary, especially as it affects understand-ability.
>
> Maybe if you proposed refactoring patches that weren't part of a dependency chain for a simple improvement then it'd be easier to have a conversation.

I think the real problem is that i'm not doing these changes because i'm trying to fill a checkbox,
or generally improve things -- the yak will *never* be fully shaven, all of this code,
will be simply replaced with new-GISel + SMT long before that can fully happen.

I'm doing these because they are blockers to another change that is blocker to another change that is blocker to another change.
It's quite clear that very few people touch this code, and even fewer understand it,
so i was hoping that i could help unblock my own patches, instead of waiting for someone else to do that for me.

So yes, it's not really likely that such refactorings would appear without bigger purpose,
moreso because the review queue is already clobbered, and it's not good to further clobber it
with changes that don't help solve bigger problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142038