llvm dev - Dec 2021 - Possible GlobalModRef bug -- arg-less calls produce wrong ref info.

Hi Nikita,

I've been tracking a miscompile in NVPTX which I've narrowed down to
this
oddity, where GlobalModRef gives different answers depending on whether an
intrinsic call has an argument or not:

https://godbolt.org/z/4PqhWKha5

The difference in behavior appears to originate here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/GlobalsModRef.cpp#L908
If the intrinsic has no arguments, it always returns `NoModRef`.
This allows LLVM to eliminate loads and stores that should have been
preserved.

I'm not sure it's the real root cause, though. Supposedly the purpose of
the `getModRefInfoForArgument` function is to tell whether the argument
references the value, so technically, if there's no argument, there's no
reference.

It's possible that the real issue is that something/somewhere ignores the
function attributes (or, rather, lack or readonly/writeonly, argmemonly,
etc) and we've just been lucky to have things working correctly for the
intrinsics *with* an argument only because `getModRefInfoForArgument` would
give a conservative answer when we use a scalar value.

I'm not familiar with AA machinery and could use your help figuring out
what's going on.

Thank you,
--Artem
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211130/9d3212b8/attachment.html>

GlobalsAAResult::getModRefInfo only analyzes globals whose address isn’t taken. 
The reasoning goes something like this: suppose you have a global which is only
loaded/stored directly.  Then the value can only be accessed by the functions
which contain those instructions, and any function that calls those functions. 
It can’t be accessed by any other function.

Looping over the arguments in getModRefInfoForArgument, as far as I can tell,
does nothing useful.  We only call into this code for globals whose address
isn’t passed to any calls, so the call’s arguments aren’t relevant: we already
know they don’t alias the global.  It looks like it’s the remains of an attempt
a few years ago to do some sort of more elaborate escape tracking in GlobalsAA.

Anyway, there’s a different hole in the logic if there’s more than one thread
involved.  If the global might be accessed from another thread, we also need to
ensure that the function doesn’t act as a synchronization barrier.  If there’s a
barrier, it could guard a modification of the value by another thread.  To fix
this hole, I think the code needs to check for a “nosync” attribute on the call.

-Eli

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Artem
Belevich via llvm-dev
Sent: Tuesday, November 30, 2021 12:01 PM
To: Nikita Popov <nikita.ppv at gmail.com>; llvm-dev <llvm-dev at
lists.llvm.org>
Subject: [llvm-dev] Possible GlobalModRef bug -- arg-less calls produce wrong
ref info.

Hi Nikita,

I've been tracking a miscompile in NVPTX which I've narrowed down to
this oddity, where GlobalModRef gives different answers depending on whether an
intrinsic call has an argument or not:

https://godbolt.org/z/4PqhWKha5

The difference in behavior appears to originate here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/GlobalsModRef.cpp#L908
If the intrinsic has no arguments, it always returns `NoModRef`.
This allows LLVM to eliminate loads and stores that should have been preserved.

I'm not sure it's the real root cause, though. Supposedly the purpose of
the `getModRefInfoForArgument` function is to tell whether the argument
references the value, so technically, if there's no argument, there's no
reference.

It's possible that the real issue is that something/somewhere ignores the
function attributes (or, rather, lack or readonly/writeonly, argmemonly, etc)
and we've just been lucky to have things working correctly for the
intrinsics *with* an argument only because `getModRefInfoForArgument` would give
a conservative answer when we use a scalar value.

I'm not familiar with AA machinery and could use your help figuring out
what's going on.

Thank you,
--Artem
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211201/1fe23bc5/attachment-0001.html>

On Tue, Nov 30, 2021 at 4:37 PM Eli Friedman <efriedma at quicinc.com>
wrote:
> GlobalsAAResult::getModRefInfo only analyzes globals whose address isn’t
> taken.  The reasoning goes something like this: suppose you have a global
> which is only loaded/stored directly.  Then the value can only be accessed
> by the functions which contain those instructions, and any function that
> calls those functions.  It can’t be accessed by any other function.
>
>
>
> Looping over the arguments in getModRefInfoForArgument, as far as I can
> tell, does nothing useful.  We only call into this code for globals whose
> address isn’t passed to any calls, so the call’s arguments aren’t relevant:
> we already know they don’t alias the global.  It looks like it’s the
> remains of an attempt a few years ago to do some sort of more elaborate
> escape tracking in GlobalsAA.
>
>
>
> Anyway, there’s a different hole in the logic if there’s more than one
> thread involved.  If the global might be accessed from another thread, we
> also need to ensure that the function doesn’t act as a synchronization
> barrier.  If there’s a barrier, it could guard a modification of the value
> by another thread.  To fix this hole, I think the code needs to check for a
> “nosync” attribute on the call.
>
After a bit of poking, I think the problem starts even earlier, in
AliasAnalysis.cpp. That's where `fence` instructions are handled (and are
treated as RefMod). Treating convergent instrinsics that may modify memory
in a similar way appears to fix the miscompile. It's probably overly
conservative, but should do the job for now.

https://reviews.llvm.org/D115302

--Artem



>
>
> -Eli
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of
*Artem
> Belevich via llvm-dev
> *Sent:* Tuesday, November 30, 2021 12:01 PM
> *To:* Nikita Popov <nikita.ppv at gmail.com>; llvm-dev <
> llvm-dev at lists.llvm.org>
> *Subject:* [llvm-dev] Possible GlobalModRef bug -- arg-less calls produce
> wrong ref info.
>
>
>
> Hi Nikita,
>
>
>
> I've been tracking a miscompile in NVPTX which I've narrowed down
to this
> oddity, where GlobalModRef gives different answers depending on whether an
> intrinsic call has an argument or not:
>
>
>
> https://godbolt.org/z/4PqhWKha5
>
>
>
> The difference in behavior appears to originate here:
>
>
>
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/GlobalsModRef.cpp#L908
>
> If the intrinsic has no arguments, it always returns `NoModRef`.
>
> This allows LLVM to eliminate loads and stores that should have been
> preserved.
>
>
>
> I'm not sure it's the real root cause, though. Supposedly the
purpose of
> the `getModRefInfoForArgument` function is to tell whether the argument
> references the value, so technically, if there's no argument,
there's no
> reference.
>
>
>
> It's possible that the real issue is that something/somewhere ignores
the
> function attributes (or, rather, lack or readonly/writeonly, argmemonly,
> etc) and we've just been lucky to have things working correctly for the
> intrinsics *with* an argument only because `getModRefInfoForArgument`
> would give a conservative answer when we use a scalar value.
>
>
>
> I'm not familiar with AA machinery and could use your help figuring out
> what's going on.
>
>
>
> Thank you,
>
> --Artem
>

-- 
--Artem Belevich
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20211207/154c969f/attachment.html>