This is an archive of the discontinued LLVM Phabricator instance.

[X86] canonicalizeShuffleWithBinOps - add handling of SSE vector shift nodes
Needs ReviewPublic

Authored by RKSimon on Jan 18 2023, 9:46 AM.

Details

Summary

This is a lighter alternative to D141877 / D141806 / D141878 to handle SSE shift nodes, to handle the case:

shuffle(shift(shuffle(x),amt1),shift(shuffle(y),amt2)) -> shift(shuffle(x,y),amt1) // iff amt1 == amt2

Diff Detail

Event Timeline

RKSimon created this revision.Jan 18 2023, 9:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 9:46 AM
RKSimon requested review of this revision.Jan 18 2023, 9:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 9:46 AM

I guess i don't see why this is better than the current version of my patches?
Also, this has insufficient test coverage.

RKSimon updated this revision to Diff 490217.Jan 18 2023, 10:10 AM
RKSimon edited the summary of this revision. (Show Details)

relax constraint to allow the shuffle to pass through the shift

lebedev.ri resigned from this revision.Jan 18 2023, 10:15 AM

I don't consider this code to be better, and if it is considered to be,
i may need to reevaluate trying to contribute to this backend.

I'm sorry if I've caused you an issue here, but you did ask on 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, 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'm sorry if I've caused you an issue here, but you did ask on 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, 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.