llvm dev - Dec 2018 - [OpenMP][AArch64][GlobalISel] AArch64 OMPT tests failing

We're seeing OMPT tests fail on AArch64:

    libomp :: ompt/misc/control_tool.c
    libomp :: ompt/synchronization/master.c
    libomp :: ompt/synchronization/taskwait.c

The failure mode is similar for all of them:

openmp/runtime/test/ompt/misc/control_tool.c:26:17: error: CHECK-NEXT: expected
string not found in input
 // CHECK-NEXT: {{^}}[[MASTER_ID]]: current_address={{.*}}[[RETURN_ADDRESS]]
                ^
<stdin>:9:1: note: scanning from here
281474976710657: current_address=0x402cf4 or 0x402cf0
^
<stdin>:9:1: note: with variable "MASTER_ID" equal to
"281474976710657"
281474976710657: current_address=0x402cf4 or 0x402cf0
^
<stdin>:9:1: note: with variable "RETURN_ADDRESS" equal to
"0x402cec"
281474976710657: current_address=0x402cf4 or 0x402cf0
^
<stdin>:9:13: note: possible intended match here
281474976710657: current_address=0x402cf4 or 0x402cf0
            ^

I bisected the control_tool.c failure to:

3834f852008a82e361d325ec7b1c3fee0dc783c3 is the first bad commit
commit 3834f852008a82e361d325ec7b1c3fee0dc783c3
Author: Petr Pavlu <petr.pavlu at arm.com>
Date:   Thu Nov 29 12:56:32 2018 +0000

    [GlobalISel] Make EnableGlobalISel always set when GISel is enabled
    
    Change meaning of TargetOptions::EnableGlobalISel. The flag was
    previously set only when a target switched on GlobalISel but it is now
    always set when the GlobalISel pipeline is enabled. This makes the flag
    consistent with TargetOptions::EnableFastISel and allows its use in
    other parts of the compiler to determine when GlobalISel is enabled.
    
    The EnableGlobalISel flag had previouly only one use in
    TargetPassConfig::isGlobalISelAbortEnabled(). The method used its value
    to determine if GlobalISel was enabled by a target and returned false in
    such a case. To preserve the current behaviour, a new flag
    TargetOptions::GlobalISelAbort is introduced to separately record the
    abort behaviour.
    
    Differential Revision: https://reviews.llvm.org/D54518
    
    
    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 347861
91177308-0d34-0410-b5e6-96231b3b80d8

Is it possible this commit changed the behavior of clang's
-fno-experimental-isel?  The OpenMP tests use that flag.

                            -David

Hi David,

I was the one who originally added the flag to fix failures related to 
GlobalISel. This was because first versions of GlobalISel didn't know 
how to select 'blockaddress', but this should have been fixed (see 
https://bugs.llvm.org/show_bug.cgi?id=36390) and available in both 7.0 
and trunk.
There has been some discussion prior to that in 
https://bugs.llvm.org/show_bug.cgi?id=36313, maybe you could look into 
what's still wrong on AArch64?

Cheers,
Jonas

On 2018-12-20 22:34, David Greene via llvm-dev wrote:
> We're seeing OMPT tests fail on AArch64:
> 
>     libomp :: ompt/misc/control_tool.c
>     libomp :: ompt/synchronization/master.c
>     libomp :: ompt/synchronization/taskwait.c
> 
> The failure mode is similar for all of them:
> 
> openmp/runtime/test/ompt/misc/control_tool.c:26:17: error: CHECK-NEXT:
> expected string not found in input
>  // CHECK-NEXT: {{^}}[[MASTER_ID]]: 
> current_address={{.*}}[[RETURN_ADDRESS]]
>                 ^
> <stdin>:9:1: note: scanning from here
> 281474976710657: current_address=0x402cf4 or 0x402cf0
> ^
> <stdin>:9:1: note: with variable "MASTER_ID" equal to
"281474976710657"
> 281474976710657: current_address=0x402cf4 or 0x402cf0
> ^
> <stdin>:9:1: note: with variable "RETURN_ADDRESS" equal to
"0x402cec"
> 281474976710657: current_address=0x402cf4 or 0x402cf0
> ^
> <stdin>:9:13: note: possible intended match here
> 281474976710657: current_address=0x402cf4 or 0x402cf0
>             ^
> 
> I bisected the control_tool.c failure to:
> 
> 3834f852008a82e361d325ec7b1c3fee0dc783c3 is the first bad commit
> commit 3834f852008a82e361d325ec7b1c3fee0dc783c3
> Author: Petr Pavlu <petr.pavlu at arm.com>
> Date:   Thu Nov 29 12:56:32 2018 +0000
> 
>     [GlobalISel] Make EnableGlobalISel always set when GISel is enabled
> 
>     Change meaning of TargetOptions::EnableGlobalISel. The flag was
>     previously set only when a target switched on GlobalISel but it is 
> now
>     always set when the GlobalISel pipeline is enabled. This makes the 
> flag
>     consistent with TargetOptions::EnableFastISel and allows its use in
>     other parts of the compiler to determine when GlobalISel is 
> enabled.
> 
>     The EnableGlobalISel flag had previouly only one use in
>     TargetPassConfig::isGlobalISelAbortEnabled(). The method used its 
> value
>     to determine if GlobalISel was enabled by a target and returned 
> false in
>     such a case. To preserve the current behaviour, a new flag
>     TargetOptions::GlobalISelAbort is introduced to separately record 
> the
>     abort behaviour.
> 
>     Differential Revision: https://reviews.llvm.org/D54518
> 
> 
>     git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 347861
> 91177308-0d34-0410-b5e6-96231b3b80d8
> 
> Is it possible this commit changed the behavior of clang's
> -fno-experimental-isel?  The OpenMP tests use that flag.
> 
>                             -David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Curious.  I removed -fno-experimental-isel and all of the tests *except*
control_tool.c passed.  I would have expected all of them to pass if
blockaddress works.

I'll try to look at some asm and see what's going on.

                         -David

Jonas Hahnfeld <hahnjo at hahnjo.de> writes:
> Hi David,
>
> I was the one who originally added the flag to fix failures related to
> GlobalISel. This was because first versions of GlobalISel didn't know
> how to select 'blockaddress', but this should have been fixed (see
> https://bugs.llvm.org/show_bug.cgi?id=36390) and available in both 7.0
> and trunk.
> There has been some discussion prior to that in
> https://bugs.llvm.org/show_bug.cgi?id=36313, maybe you could look into
> what's still wrong on AArch64?
>
> Cheers,
> Jonas
>
> On 2018-12-20 22:34, David Greene via llvm-dev wrote:
>> We're seeing OMPT tests fail on AArch64:
>>
>>     libomp :: ompt/misc/control_tool.c
>>     libomp :: ompt/synchronization/master.c
>>     libomp :: ompt/synchronization/taskwait.c
>>
>> The failure mode is similar for all of them:
>>
>> openmp/runtime/test/ompt/misc/control_tool.c:26:17: error: CHECK-NEXT:
>> expected string not found in input
>>  // CHECK-NEXT: {{^}}[[MASTER_ID]]:
>> current_address={{.*}}[[RETURN_ADDRESS]]
>>                 ^
>> <stdin>:9:1: note: scanning from here
>> 281474976710657: current_address=0x402cf4 or 0x402cf0
>> ^
>> <stdin>:9:1: note: with variable "MASTER_ID" equal to
"281474976710657"
>> 281474976710657: current_address=0x402cf4 or 0x402cf0
>> ^
>> <stdin>:9:1: note: with variable "RETURN_ADDRESS" equal
to "0x402cec"
>> 281474976710657: current_address=0x402cf4 or 0x402cf0
>> ^
>> <stdin>:9:13: note: possible intended match here
>> 281474976710657: current_address=0x402cf4 or 0x402cf0
>>             ^
>>
>> I bisected the control_tool.c failure to:
>>
>> 3834f852008a82e361d325ec7b1c3fee0dc783c3 is the first bad commit
>> commit 3834f852008a82e361d325ec7b1c3fee0dc783c3
>> Author: Petr Pavlu <petr.pavlu at arm.com>
>> Date:   Thu Nov 29 12:56:32 2018 +0000
>>
>>     [GlobalISel] Make EnableGlobalISel always set when GISel is enabled
>>
>>     Change meaning of TargetOptions::EnableGlobalISel. The flag was
>>     previously set only when a target switched on GlobalISel but it
>> is now
>>     always set when the GlobalISel pipeline is enabled. This makes
>> the flag
>>     consistent with TargetOptions::EnableFastISel and allows its use in
>>     other parts of the compiler to determine when GlobalISel is
>> enabled.
>>
>>     The EnableGlobalISel flag had previouly only one use in
>>     TargetPassConfig::isGlobalISelAbortEnabled(). The method used
>> its value
>>     to determine if GlobalISel was enabled by a target and returned
>> false in
>>     such a case. To preserve the current behaviour, a new flag
>>     TargetOptions::GlobalISelAbort is introduced to separately
>> record the
>>     abort behaviour.
>>
>>     Differential Revision: https://reviews.llvm.org/D54518>
>>
>>     git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at
347861> 91177308-0d34-0410-b5e6-96231b3b80d8
>>
>> Is it possible this commit changed the behavior of clang's
>> -fno-experimental-isel?  The OpenMP tests use that flag.
>>
>>                             -David
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev