RFR: 8151661: Performance regression on Solaris-SPARC in 9-b103

[Vladimir Kozlov]

Hi Andrew,

 > the predicates.  It makes no sense at all for short and long variants
 > to have different predicates, so suspect this is a bug in adlc.

I think this statement is not correct as SPARC problem shows.
New branch instructions could be introduced on new CPU (cbcond in this 
case) and it usage will be guarded by predicate in .ad file. In such 
case predicates for long and short branches will be different.

I also understand your case when you use predicate to check condition 
code. Yes, in such case you have to match predicates. But the same 
result you can get with specialized cmpOp operands. And specialized immI 
(for op2) when you check is_power_of_2().

Regards,
Vladimir

On 6/8/16 1:22 AM, Andrew Haley wrote:
> On 08/06/16 08:14, Rahul Raghavan wrote:
>
>> 4. http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2015-December/020450.html
>
>> From this old 8145438 - RFR email thread, seems the adlc change was
>> done independently, not as an integral part of original
>> 8145438/8144028 -fix.
>
>> The same change was done based on the assumption that short and long
>> branch variants cannot have different predicates.
>
>> But this is wrong for above SPARC cases.
>
>> Reverting only the adlc change, could not find any issues with -
>>     JPRT testing (default -testset hotspot) , 'hotspot/test/compiler/codegen/8144028/BitTests.java' jtreg test run for Sparc, AArch64)
>>
>> So in general predicates for short and long branches could be
>> different and should not be compared for check_branch_variant()!
>
>> The above proposed webrev.00 fix proposal is the same reverting
>> earlier 8145438 adlc change.
>
> That just re-introduces the bug.  check_branch_variant() is suposed to
> test whether one branch can be replaced by another, and the predicate
> may be an important part of that.  For example, in the case of AArch64
> some predicates test if an operand is a power of two: only then may a
> bit test and branch instruction be used.  The versions of branch with
> and without this test are not semantically equivalent, so one branch
> cannot be replaced by another.
>
> I wonder if there is a better way to solve this problem without
> weakening check_branch_variant().  The fact that it didn't check
> predicates to see if one branch can be replaced by another was a bug.
> It was a useful bug, sure, but still a bug.
>
> We could -- in extremis -- #ifdef AARCH64 in check_branch_variant(),
> but it would be better to find another way to do it.  For example, we
> could define a semantic_predicate and use that.  That way we would
> separate predicates which change the meaning of patterns from those
> which merely enable them based on some global flag.
>
> Andrew.
>