RFR (S): 8198561: Make oop and narrowOop always have their own class type

[Erik Österlund]

Hi Kim,

Thank you for looking at this.

New full webrev covering all comments so far (hopefully):
http://cr.openjdk.java.net/~eosterlund/8198561/webrev.01/

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8198561/webrev.00_01/

On 2018-02-27 00:21, Kim Barrett wrote:
>> On Feb 26, 2018, at 8:32 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi,
>>
>> Making oop sometimes map to class types and sometimes to primitives comes with some unfortunate problems. Advantages of making them always have their own type include:
>>
>> 1) Not getting compilation errors in configuration X but not Y
>> 2) Making it easier to adopt existing code to use Shenandoah equals barriers
>> 3) Recognize oops and narrowOops safely in template
>>
>> Therefore, I would like to make both oop and narrowOop always map to a class type consistently.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8198561/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8198561
>>
>> Thanks,
>> /Erik
> -----
> Why is narrowOop::_value public?

That was simply a mistake. I have corrected it to be private now. Thank 
you for catching that.

> -----
> 54   narrowOop& operator=(const narrowOop o)    { _value = o._value; return *this; }
> 55   narrowOop& operator=(const PrimitiveType value) { _value = value; return *this; }
>
> Given we have the conversion from PrimitiveType, do we need the assignment from PrimitiveType?
> That wouldn’t permit direct assignment if the conversion were made explicit (which I think it should),
> but then, I’m not sure that assignment from a bare PrimitiveType is really a good idea either.

You are right - we seem to be fine without assignment from 
PrimitiveType. I removed it.

> -----
> 45   narrowOop(const PrimitiveType value) : _value(value) {}
>
> Should this conversion be explicit?  And should it permit implicit narrowing integral conversions?
> The narrowing conversions could be poisoned, though that’s a bit uglier for a constructor than for
> the other operations (see below).

Since narrowOop used to be a juint, there is a bunch of code that relies 
on this conversion being implicit for now. So I think I would prefer to 
keep this implicit to reduce fanout. I tried making it explicit, and 
unfortunately changes required for that propagate surprisingly far.

> -----
> All the narrowOop operations on PrimitiveType will permit implicit narrowing integer conversions
> to the PrimitiveType. That doesn’t seem like such a good idea.  The narrowing conversions could
> be poisoned.

That is true. I managed to get rid of that in the latest revision.

> -----
>
> src/hotspot/cpu/sparc/relocInfo_sparc.cpp
> 99       uint32_t np = type() == relocInfo::oop_type ? (uint32_t)oopDesc::encode_heap_oop((oop)x) : Klass::encode_klass((Klass
> 100       inst &= ~Assembler::hi22(-1);
> 101       inst |=  Assembler::hi22((intptr_t)(uintptr_t)np);
>
> (1) Some text seems to have been lost at the end of line 99.  I suspect this doesn’t compile.

This seems to be a tooling problem that my webrev tool cuts the text 
after 125 characters. Sorry about that. This seems to indicate though 
that perhaps that line is too long anyway, so I split it into two lines.

> (2) In old code, np was of type jint, and was just cast to intptr_t.  Both value clauses in the initializer
> return 32bit unsigned values.  If the high bit of of the value can be set, then the value passed to hi22
> will differ between the old code and the new.

Yes you are right. I thought sign extending a narrowOop to an int64_t 
value seemed like a bad idea and that if it produced different values, I 
would never want to sign extend the narrowOop. After looking closer 
though, the expected argument to hi22 is an int. So the whole sign 
extending cast to intptr_t thing seems pointless anyway and would result 
in the same int you already had. I changed it to an int and removed 
subsequent explicit casts to intptr_t that then implicitly got converted 
back to int (that it already was declared as).

Thanks,
/Erik