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

[Vladimir Kozlov]

On 3/30/20 9:04 AM, Erik Österlund wrote:
> 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.

Okay.

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

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

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

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