[PATCH] D78272: [PowerPC] DAG Combine to transform shifts into multiply-high

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D78272#2029885 <https://reviews.llvm.org/D78272#2029885>, @nemanjai wrote:

> In D78272#2026458 <https://reviews.llvm.org/D78272#2026458>, @craig.topper wrote:
>
> > In D78272#2025841 <https://reviews.llvm.org/D78272#2025841>, @amyk wrote:
> >
> > > @craig.topper Do you think common-ing out the X86/PPC parts to combine to mulh into the target independent combiner is a good idea?
> >
> >
> > I see a few issues.
> >
> > X86 doesn't have scalar MULHU/MULHS instructions, but we have vector MULHU/MULHS on vXi16. We currently match from truncate rather than from shift. I tried to move it to shift following your code here, but I got regressions. Primarily because we handled the truncate first and turned into PACKSS/PACKUS, may an AND or SIGN_EXTEND_INREG, and some subvector extracts.. Then we match the MULH, but we couldn't fold away the sext/zext and what we had turned the truncate into. We might be able to improve that.   I think it is useful to match from the shift since the truncate won't always be there. So we might need matching from both.
>
>
> It sounds like if we were to put this code into target-independent DAG Combine, you shouldn't see similar regressions since your combines will still run and this patch might pick up some of what couldn't be combined. Or am I misinterpreting your comment?


Yes our combine will still run. I have no issue putting this in target-independent combine. I was answering from the position of why putting it in target combine doesn't get rid of X86 specific code. Which I assume was at least part of @RKSimon's motivation for moving it.

>> The other issue is that for vectors we need to match for vectors width more than the legal number of elements before type legalization. Otherwise the extends we match get type legalized into something much harder to match. But checking isOperationLegal won't work for that. Maybe we could walk the type legalization steps to find what it would be legalized to?
> 
> If I am not mistaken, this runs before legalization as well so it should combine something like
>  `(srl (mul (zext v16i16:$a to v16i32), (zext v16i16:$a to v16i32)), (splat 16 to v16i32))`
>  As long as `MULH` is legal for `v16i16` (even though `v16i32` is not a legal type). Of course, the actual types just for illustration - not to suggest that those are the actual types for any specific target.
>  Or am I again not reading your comment correctly?

But it's still probably worthwhile for illegal types like v128i16 since it turns two extends on the inputs to one on the output. Type legalization will split the v128i16 MULHU/MULHS. This needs to be matched before type legalization makes the v128i32 zero extend hard to spot. Number of elements exaggerated to make it look it likely illegal.


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

https://reviews.llvm.org/D78272