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

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

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