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

[Erik Österlund]

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.

>>
>>> 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/

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
>>>>
>>