Page MenuHomePhabricator

AArch64+ARM: make LLVM consider system registers volatile to prevent unsound optimizations.
AcceptedPublic

Authored by t.p.northover on Jun 1 2020, 2:16 AM.

Details

Summary

Some of the system registers readable on AArch64 & ARM platforms return different values with each read (for example a timer counter), these shouldn't be hoisted outside loops or otherwise interfered with, but the normal @llvm.read_register intrinsic is only considered to read memory.

This introduces a separate @llvm.read_volatile_register intrinsic and maps all system-registers on ARM platforms to use it for the __builtin_arm_rsr calls. Registers declared with asm("r9") or similar are unaffected.

Diff Detail

Event Timeline

t.p.northover created this revision.Jun 1 2020, 2:16 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 1 2020, 2:16 AM
fhahn added a subscriber: fhahn.

This seems similar to the problem described in https://bugs.llvm.org/show_bug.cgi?id=46210 : for intrinsics that are marked as reading memory, we do not account for the fact that the memory could be modified outside of the current program, which is what happens here.

The intrinsic seems like a good approach for addressing the issue for llvm.read_register incrementally. I guess there are other instances where we actually want llvm.read_volatile_register on other platforms as well.

As an alternative, and necessary regardless, we need to teach AA that "no-nosync" operations can experience the effects of external things. As long as llvm.read_register is not marked nosync that should prevent hoisting. Should, because AA doesn't know this yet. (https://bugs.llvm.org/show_bug.cgi?id=46210).

I do however see the potential for two intrinsics here, one that is a synchronizing read and one that is not. From that perspective this seems reasonable.

fhahn accepted this revision.Fri, Jul 3, 2:01 AM

As mentioned earlier, I think the patch is a nice, isolated fix for the read_register issue and it should not make addressing the bigger issue more difficult.

LGTM, thanks!

This revision is now accepted and ready to land.Fri, Jul 3, 2:01 AM