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

[Erik Österlund]

Hi Vladimir,

Thank you for reviewing.

On 2020-03-24 18:44, Vladimir Kozlov wrote:
> 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?

No. The flushing of the bundle was not performed, but the padding was. 
But flushing the bundle on x86 is a no-op,
because we don't have things like delay slots to worry about. So only 
the padding is really important here. And that
was done. With this new approach that tries to hide the platform 
specific code, we will get calls to flush the bundle
att jcc erratum nodes, which is harmless as it does not really do 
anything on x86.

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

I ran a bunch of things with -XX:+CITime, and compared the code emission 
(PhaseOutput) times compared to the overall
compilation time.The percentage is stable both with and without 
-XX:IntelJccErratumMitigation, with any difference
in the noise (PhaseOutput ~5.7% +/- 0.1% oftotal C2 compilation times). 
Since the whole code emission phase performs
the same with and without the mitigation, my analysis is that theslower 
code only observed with
-XX:-IntelJccErratumMitigation could not possiibly have any noticeable 
effect.

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

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