[14] RFR (XS): 8234392: C2: Extend Matcher::match_rule_supported_vector() with element type information

[John Rose]

Two non-grumpy thumbs up!

(Braces or not, either way, that’s up to Vladimir K.)

> On Dec 11, 2019, at 3:55 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> 
>> Yes, fully agree. Updated version:
>>   http://cr.openjdk.java.net/~vlivanov/8234392/webrev.01/
>> Got rid of ret_value and enhanced the comments, but also fixed long-standing bug you noticed: AbsVF should have the same additional checks as NegVF (since 512bit vandps is also introduced in AVX512DQ).
> 
> Additionally, got rid of ret_value in Matcher::match_rule_supported() and moved Op_RoundDoubleModeV there since it doesn't depend on vector length:
> 
> +    case Op_RoundDoubleModeV:
> +      if (VM_Version::supports_avx() == false) {
> +        return false; // 128bit vroundpd is not available
> +      } break;
> 
> Best regards,
> Vladimir Ivanov
> 
>> On 11.12.2019 02:31, John Rose wrote:
>>> Actually I have one more comment about the new classification logic:
>>> 
>>> The “ret_value” idiom is terrible.
>>> 
>>> I see a function which is complex, with something like this at the top:
>>> 
>>>    if (… simple size check …) {
>>>      ret_value = false;   // size not supported
>>>    } …
>>> 
>>> I want to read something more decisive like this:
>>> 
>>>    if (… simple size check …) {
>>>      return false;   // size not supported
>>>    } …
>>> 
>>> The “ret_value” thingy adds only noise, and no clarity.
>>> 
>>> It is more than an annoyance, hence my comment here.
>>> The problem is that if I want to understand the quick check
>>> above, I have to scroll down *past all the other checks* to see
>>> if some joker does “ret_value = true” before the “return ret_value”,
>>> subverting my understanding of the code.  Basically, the “ret_value”
>>> nonsense makes the code impossible to break into understandable
>>> parts.
>>> 
>>> — Grumpy John
>>> 
>>> On Dec 10, 2019, at 12:34 PM, John Rose <john.r.rose at oracle.com <mailto:john.r.rose at oracle.com>> wrote:
>>>> 
>>>>> It would severely penalize Xeon Phis (which lack BW, DQ, and VL extensions), but maybe there'll be a moment when Skylake (F+CD+BW+DQ+VL) can be chosen as the baseline.
>>>> 
>>>> Yeah, I saw that coming after I visited the trusty intrinsics guide
>>>> https://software.intel.com/sites/landingpage/IntrinsicsGuide/
>>>>> 
>>>>> Anyway, I got your idea and it makes perfect sense to me to collect such ideas.
>>>> 
>>>