RFR: 8351468: C2: array fill optimization assigns wrong type to intrinsic call

[Emanuel Peter]

On Fri, 14 Mar 2025 11:51:51 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>>> I would expect the `memory_type` of a `StoreC` into a `long[]` to be something that means "a part of a `long[]`"
>> 
>> If that was the intended meaning of `MemNode::memory_type()`, wouldn't the function be redundant, because we can retrieve that information from `MemNode::adr_type()` already?
>
> @robcasloz Yes that's right. Then `MemNode::memory_type()` does not refer to the thing in memory at all, but the thing that is about to interact with the memory. I think:
> 
> - We should rename it to `MemNode::value_type()` or `MemNode::value_basic_type()`
> - It is simply incorrect to use it to reason about the thing in the memory in this problem, and using `adr_type` is the correct fix.
> 
> To be clear, I don't think having `StoreSNode` would solve any issue. I can `StoreS` into a `char[]`, and `StoreC` into a `short[]` and we are back at the same issue.

@merykitty You are right, `MemNode::memory_type` is very easy to misunderstand. We could probably rename it, and while doing that check all usages. We have had bugs like this before, I think I had one in SuperWord as well some years ago... What would be a better name though?

Quickly looking at the cases, there are not even that many usages:

emanuel at emanuel-oracle:/oracle-work/jdk-fork0/open$ grep memory_type src/hotspot/share/opto/ -r
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const = 0;
src/hotspot/share/opto/memnode.hpp:    return type2aelembytes(memory_type(), true);
src/hotspot/share/opto/memnode.hpp:    return type2aelembytes(memory_type());
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_BYTE; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_BYTE; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_CHAR; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_SHORT; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_INT; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_LONG; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_FLOAT; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_DOUBLE; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_ADDRESS; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_NARROWOOP; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_NARROWKLASS; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_BYTE; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_CHAR; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_INT; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_LONG; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_FLOAT; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_DOUBLE; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_ADDRESS; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_NARROWOOP; }
src/hotspot/share/opto/memnode.hpp:  virtual BasicType memory_type() const { return T_NARROWKLASS; }
src/hotspot/share/opto/escape.cpp:          // StoreP::memory_type() == T_ADDRESS
src/hotspot/share/opto/escape.cpp:              store->as_Store()->memory_type() == ft) {
src/hotspot/share/opto/vectornode.hpp:  virtual BasicType memory_type() const { return T_VOID; }
src/hotspot/share/opto/vectornode.hpp:  virtual BasicType memory_type() const { return T_VOID; }
src/hotspot/share/opto/memnode.cpp:      if (memory_type() != T_VOID) {
src/hotspot/share/opto/memnode.cpp:          return phase->zerocon(memory_type());
src/hotspot/share/opto/memnode.cpp:                                                                      memory_type(), is_unsigned());
src/hotspot/share/opto/memnode.cpp:      const Type* con_type = Type::make_constant_from_field(const_oop->as_instance(), off, is_unsigned(), memory_type());
src/hotspot/share/opto/superword.cpp:      bt = n->as_Mem()->memory_type();
src/hotspot/share/opto/superword.cpp:        bt = n->as_Mem()->memory_type();
src/hotspot/share/opto/superword.cpp:        is_java_primitive(mem->memory_type())) {
src/hotspot/share/opto/superword.cpp:  if (!is_java_primitive(s1->as_Mem()->memory_type()) ||
src/hotspot/share/opto/superword.cpp:      !is_java_primitive(s2->as_Mem()->memory_type())) {
src/hotspot/share/opto/superword.cpp:    BasicType bt = n->as_Mem()->memory_type();
src/hotspot/share/opto/loopTransform.cpp:  BasicType t = store->as_Mem()->memory_type();
src/hotspot/share/opto/loopTransform.cpp:        if (type2aelembytes(store->as_Mem()->memory_type(), true) != (1 << n->in(2)->get_int())) {
src/hotspot/share/opto/loopTransform.cpp:  BasicType t = store->as_Mem()->memory_type();


Well, I looked through them, and I cannot see any issue with the other cases. But maybe someone else can give the usages a quick look too.

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

PR Comment: https://git.openjdk.org/jdk/pull/24005#issuecomment-2742835812