RFR: 8241438: Move IntelJccErratum mitigation code to platform-specific code

[Vladimir Kozlov]

On 4/1/20 3:24 AM, Erik Österlund wrote:
> Hi Vladimir,
> 
> On 2020-03-30 21:14, Vladimir Kozlov wrote:
>> But you at least can do static check at the beginning of method:
>>
>> int MachNode::pd_alignment_required() const {
>>   if (VM_Version::has_intel_jcc_erratum()) {
>>     PhaseOutput* output = Compile::current()->output();
>>     Block* block = output->block();
>>     int index = output->index();
>>     assert(output->mach() == this, "incorrect iterator state in PhaseOutput");
>>     if (IntelJccErratum::is_jcc_erratum_branch(block, this, index)) {
>>       // Conservatively add worst case padding. We assume that relocInfo::addr_unit() is 1 on x86.
>>       return IntelJccErratum::largest_jcc_size() + 1;
>>     }
>>   }
>>   return 1;
>> }
> 
> That is equivalent to the compiler. I verified that by disassembling the release bits before
> and after your suggestion, and it is instruction by instruction the same. In both cases it
> first checks ifVM_Version::has_intel_jcc_erratum(), and if not, returns before even building
> a frame. I'd rather keep the not nested variant because it is equivalent, yet easier to read.

I have reservation about this statement which may not true for all C++ compilers we use but I will not insist on 
refactoring it.

> 
>>>
>>>> In compute_padding() reads done under check so I have less concerns about it. But I also don't get why you use saved 
>>>> _mach instead of using MachNode 'this'.
>>>
>>> Good point. I changed to this + an assert checking that they are indeed the same.
>>
>> Why do you need Output._mach at all if you use it only in this assert? Even logically it looks strange. In what case 
>> it could be different?
> 
> It should never be different; that was the point. The index and mach node exposed by the
> iterator are related and refer to the same entity. So if you use the exposed index in code
> in a mach node, you must know that this mach node is the same mach node that the index refers
> to, and it is. The assert was meant to enforce it so that if you were to call either the
> alignment or padding function in a new context, for whatever reason, and don't happen to know
> that you can't do that without having a consistent iteration state, you would immediately catch
> that in the assertions, instead of getting strange silent logic errors.
> 
> Having said that, I am okay with removing _mach if you prefer having one seat belt less, it is up to you:
> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.03/

Okay. Good.

Thanks,
Vladimir

> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.02_03/
> 
> Thanks,
> /Erik
> 
>> Thanks,
>> Vladimir
>>
>>>
>>> Here is an updated webrev with your concerns and Vladimir Ivanov's concerns addressed:
>>> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.02/
>>>
>>> Incremental:
>>> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.01_02/
>>>
>>> Thanks,
>>> /Erik
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>>> In pd_alignment_required() you implicitly use knowledge that relocInfo::addr_unit() on x86 is 1.
>>>>>> At least add comment about that.
>>>>>
>>>>> I can add a comment about that.
>>>>>
>>>>> New webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.01/
>>>>>
>>>>> Incremental:
>>>>> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.00_01/
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 3/23/20 6:09 AM, Erik Österlund wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> There is some platform-specific code in PhaseOutput that deals with the IntelJccErratum mitigation,
>>>>>>> which is ifdef:ed in shared code. It should move to platform-specific code.
>>>>>>>
>>>>>>> This patch exposes the iteration state of PhaseOutput, which allows hiding the Intel-specific code
>>>>>>> completely in x86-specific files.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~eosterlund/8241438/webrev.00/
>>>>>>>
>>>>>>> Bug:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8241438
>>>>>>>
>>>>>>> Thanks,
>>>>>>> /Erik
>>>>>
>>>
>