[PATCH] D68651: [InstCombine] Signed saturation patterns

Sat Oct 12 14:16:05 PDT 2019

dmgreen added a comment.

In D68651#1703888 <https://reviews.llvm.org/D68651#1703888>, @dmgreen wrote:

> At some point when we get into enough of the details the code becomes the best documentation, but I'm guessing you will disagree with that...

First up, sorry about this line. I was in a bit of a mood over emails not working and someone distracted me, I pressed submit before I had properly constructed what I really wanted to say. Sorry if it came across as accusatory or passive aggressive. I was really wanting to say that; strong specifications, whilst they can be very useful, can also be stifling to innovation and bog down forward progress. We should try to keep a balance between writing things down and getting things done :)

Secondly, I had to revert D68643 <https://reviews.llvm.org/D68643> because it wasn't sign extending the values as it should. Simple cases were working, but more complex examples were not. This will mostly effect illegal types (like i4's or i8/i16 on arm), but will make the results look less impressive. I will put up another patch together showing the correct results, which will contain more extends in places. I don't think it changes the decision of whether to use the intrinsics, just moves a few cases from looking like "an improvement", to just being "the same". And still better for many cases like vectorisation.

In D68651#1706810 <https://reviews.llvm.org/D68651#1706810>, @hsaito wrote:

> I don't want to hijack this review. Let me have the rest of the discussion in the form of RFC on llvm-dev. Vector idiom discussions resulted in ~20 idioms. It would be nice if we can come up with a basic guideline on how to think and how to make a case for the new canonical form. For example, TI said they are interested in saturating mul. If saturating add/sub have a canonical form, I don't see why saturating mul should not.

I was going to say yeah, sounds great to me, let me look into adding it.. But I took another look at the MVE spec and we apparently don't have that instruction. There are some that do multiplying + doubling and saturate the high half. Apparently that's useful to someone! I don't believe it would be a detriment to have a saturating mul though, so sounds good to me if someone wants to add it.

Do you happen to know if that discussion you spoke of was written down? It sounds very interesting. One thing I would like to try is to kick llvm's use of reductions up to 11, and it might provide useful insights on the best way to make that work.

In D68651#1707017 <https://reviews.llvm.org/D68651#1707017>, @nikic wrote:

> I'm only wondering whether the trunc is the right place to start the match. Starting from the min/max we could match a larger set of patterns, in particular those where the result of the saturation is still extended to a larger type -- for example doing a 16-bit saturating add but continuing with a 32-bit result.

Yeah I was wondering if that would be better. So long as it doesn't create strange types, I think it sounds like a good idea. I'll give it a try and let you know how it looks.

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

https://reviews.llvm.org/D68651