RFR: 8257483: C2: Split immediate vector rotate from RotateLeftV and RotateRightV nodes [v4]

[Dong Bo]

On Thu, 3 Dec 2020 17:48:12 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> Yes, it works just the same as you mentioned. Thanks a lot to make this so clear.
>> The degeneration to `OrV+LShiftV+URShiftV` is at `RotateLeftVNode::Ideal()`, then `VectorNode::degenerate_vector_rotate()`:
>> `RotateLeft ==vectorizes==> RotateLeftV    ==degenerates(RotateLeftVNode::Ideal, VectorNode::degenerate_vector_rotate())==> OrV+LShiftV+URShiftV`
>> 
>> However, the workflow relys on the creation of `RotateLeftV` node, and the creation of `RotateLeftV` with variables should be forbidden if one CPU match RotateLeftV/RotateRightV only with constant shifts.
>> Otherwise, `Matcher::Label_Root()` will check wether the AD files have implemented the matching rules for RotateLeftV with variables.
>> If there are no rules for `RotateLeftV` with variables, while we have rules for `RotateLeftV` with constants, we get a crash at https://github.com/openjdk/jdk/blob/fa58671f9f8a8230d907c0a6289685f8455bce1f/src/hotspot/share/opto/matcher.cpp#L1669.
>> 
>> IMHO, seems ajusting checks in `RotateLeftV::Ideal` does not deliver the solution, since there are no `RotateRightV` for variable shifts from the start.
>
>> However, the workflow relys on the creation of RotateLeftV node, and the creation of RotateLeftV with variables should be forbidden if one CPU match RotateLeftV/RotateRightV only with constant shifts.
> 
> But degeneration step is specifically designed to cover the case when rotate vector node is not supported by the matcher.
> It is perfectly fine to instantiate RotateV node considering it will be replaced by OrV+LShiftV+URShiftV before matcher kicks in. You just need to adjust the check so it works properly, don't you?

Sorry, my mistake, ajusting the checks would be enough.

Seems we have to pass the `in(2)->is_Con()` to code implemented by a CPU, so that the CPU can return wether the CPU supports `RotateRightV` and `RotateLeftV` variables or not.
I extended the exsiting `bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType bt)` to `bool match_rule_supported_vector(int opcode, int vlen, BasicType bt, int ext = MATCH_RULE_SUPPORT_VECTOR_HAS_NO_EXTINFO)`.
This function works the same as before if the new added `ext` equals to default `MATCH_RULE_SUPPORT_VECTOR_HAS_NO_EXTINFO`.
In this case, the new added parameter `ext` is supposed to indicate the shift is a `constant` or a `variable` for `RotateRightV` and `RotateLeftV`.
We can handle it at the start of when neccessary, for optimizing `RotateRightV` and `RotateLeftV` on aarch64, like:
-const bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType bt) {
+const bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType bt, int ext) {
+  if (Matcher::match_rule_supported_vector_has_extinfo(ext)) {
+    switch(opcode) {
+      case Op_RotateRightV:
+      case Op_RotateLeftV:
+        if (VectorNode::is_rotate_vector_var(ext)) {
+          return false;
+       }
+        break;
+      default:
+        break;
+    }
+  }

With this, I think it is also easy to extended to cover other possible special  CPU dependent checks for match rules in the future, like partially supports for other `opcode`.

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

PR: https://git.openjdk.java.net/jdk/pull/1532