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

[Erik Österlund]

Hi Vladimir,

Thanks for the review.

/Erik

On 2020-04-02 04:57, Vladimir Kozlov wrote:
> 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
>>>>>>
>>>>
>>