[jdk18] RFR: 8278267: ARM32: several vector test failures for ASHR [v3]

[Dean Long]

On Tue, 4 Jan 2022 04:02:03 GMT, Hao Sun <haosun at openjdk.org> wrote:

>> src/hotspot/cpu/arm/arm.ad line 10795:
>> 
>>> 10793: 
>>> 10794: instruct vsl8B_immI(vecD dst, vecD src, immI shift) %{
>>> 10795:   predicate(n->as_Vector()->length() == 8 && !n->as_ShiftV()->is_var_shift());
>> 
>> Is checking for is_var_shift() really necessary for these immI rules?  It seems like it should never happen, so it could be an assert.
>
> An assertion should be better, but I didn't find a proper way to add one assertion for `instruct` according to `src/hotspot/share/adlc/Doc/Syntax.doc`.
> 
> Hence I'd like to put this check to `predicate`, and "bad AD file` assertion would be issued for unexpected cases.

You can put the assert into a helper function.  You can look in aarch64.ad for examples.

bool assert_not_var_shift(const Node *n) {
  assert(!n->as_ShiftV()->is_var_shift(), "illegal var shift");
  return true;
}

[...]

predicate(n->as_Vector()->length() == 8 && assert_not_var_shift(n));

>> src/hotspot/share/opto/vectornode.hpp line 546:
>> 
>>> 544:   virtual bool cmp(const Node& n) const {
>>> 545:     return VectorNode::cmp(n) && _is_var_shift == ((ShiftVNode&)n)._is_var_shift;
>>> 546:   }
>> 
>> Is it important to add hash() and cmp()?
>
> I think it's safe to add them to avoid mis-optimizations at IR level.
> 
> Per my understanding, two ShiftV nodes with the same vector elements but with different `_is_var_shift` field values, are two different nodes, and they cannot be merged/treated as the same node.

That makes sense.  Is it possible to write a test for that?

-------------

PR: https://git.openjdk.java.net/jdk18/pull/41