This is an archive of the discontinued LLVM Phabricator instance.

[ARM] fpscr read/write intrinsics not aware of each other.
ClosedPublic

Authored by rs on Mar 2 2017, 10:16 AM.

Details

Summary

The intrinsics builtin_arm_get_fpscr and builtin_arm_set_fpscr read from and write to the fpscr (Floating-Point Status and Control Register) register.

A translation fault occurs if there is a read then write then read from this register:

void foo(int *p) {
     p[0] = __builtin_arm_get_fpscr();
     __builtin_arm_set_fpscr(1);
     p[1] = __builtin_arm_get_fpscr();
}

The generated code for the above example only has one read and one write the second read is common sub-expression eliminated into the first read. This is obviously wrong. I think the reason this is happening is due to the intrinsic definition in IntrinsicsARM.td for arm_get_fpscr which has IntrNoMem:

def int_arm_get_fpscr : GCCBuiltin<"__builtin_arm_get_fpscr">,
Intrinsic<[llvm_i32_ty], [], [IntrNoMem]>;

the definition for IntrNoMem is:

// IntrNoMem - The intrinsic does not access memory or have any other
// side effects.  It may be CSE'd deleted if dead, etc.
def IntrNoMem : IntrinsicProperty;

The problem is that LLVM doesn't realize it's reading the register that __builtin_arm_set_fpscr wrote. So a potential fix for this problem could be to treat a read of fpscr as a memory access the same way as a write is treated as a memory access.

Diff Detail

Event Timeline

rs created this revision.Mar 2 2017, 10:16 AM
rs edited the summary of this revision. (Show Details)Mar 2 2017, 10:23 AM
jmolloy accepted this revision.Mar 3 2017, 1:46 AM

Hi Ranjeet,

This looks correct to me - modelling sideeffects as memory reads/writes is fairly standard. However, in your testcase you appear to be using attribute sets that aren't defined (#0, #1, #2) - does that actually compile? They should be removed nevertheless.

Otherwise LGTM.

James

This revision is now accepted and ready to land.Mar 3 2017, 1:46 AM
rs updated this revision to Diff 90450.Mar 3 2017, 3:14 AM
rs added a comment.Mar 3 2017, 3:16 AM

Thanks for reviewing James.

. However, in your testcase you appear to be using attribute sets that aren't defined (#0, #1, #2) - does that actually compile? They should be removed nevertheless.

I've gone ahead and removed the build attribute numbers, it did compile though. I've also updated the test to run at different optimization levels.

LGTM, please commit.

This revision was automatically updated to reflect the committed changes.