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

[Vladimir Kozlov]

Hi Erik

On 3/24/20 9:37 AM, Erik Österlund wrote:
> Hi Vladimir,
> 
> Thank you for the review.
> 
> On 2020-03-23 21:18, Vladimir Kozlov wrote:
>> Hi Erik
>>
>> Why you calling Compile::current() in PhaseOutput()?
>> Base class Phase has C value set already.
> 
> Oops, I accidentally got that line from a merge conflict. My bad. Will remove that line.
> 
>> I don't think you can use virtual method compute_padding() as you do. Call nodes and other have own versions.
> 
> I already in the current tip assert that there is not conflicting paddings between what the node wants
> what the JCC node wants. If there was such an issue, we would have asserted by now. For example, Java calls
> have padding due to cross-modifying code. But that same padding makes it JCC-safe, so the JCC padding code
> will not request a different padding.Therefore it is safe to do it this way.

Okay. I verified that CallStaticJavaDirectNode and CallDynamicJavaDirectNode which have own alignment and padding were 
not included in JCC padding code. So it seems fine as you said.

With your changes next code is effected now:
http://hg.openjdk.java.net/jdk/jdk/file/6ff2d38ef833/src/hotspot/share/opto/output.cpp#l1419

Did we missed it in original changes?

> 
>> The same for alignment_required().
> 
> And same as I said above. ;)
> 
>> I would suggest instead to add PhaseOutput platform specific methods and pass Node* parameter.
> 
> Do you still suggest that after my explanations above that the alignment/padding requrement is already
> asserted to not be conflicting between JCC code and individual mach nodes?

I have an other concern about pd_alignment_required(). It does indirect reads before checks and it may affect 
performance since it is done for all nodes.
May be we should manually inline checks from is_jcc_erratum_branch() or do as I suggested.

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

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
>