Page MenuHomePhabricator

ARM: fix VFP asm constraints
ClosedPublic

Authored by jfb on Feb 17 2016, 2:14 PM.

Details

Summary

Rich Felker was sad that clang used 'w' and 'P' for VFP constraints when GCC documents them as 't' and 'w':

https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html

This was added way back in 2008:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20080421/005393.html

Diff Detail

Repository
rL LLVM

Event Timeline

jfb updated this revision to Diff 48238.Feb 17 2016, 2:14 PM
jfb retitled this revision from to ARM: fix VFP asm constraints.
jfb updated this object.
jfb added a subscriber: cfe-commits.

Test cases? It seems that 't' and 'w' are not available in thumb-1 mode.

jfb updated this revision to Diff 48363.Feb 18 2016, 10:56 AM
  • Add test.
jfb added a comment.Feb 18 2016, 11:11 AM

Added a test.

Constraint validation is lacking in LLVM: it doesn't check that the input variable matches the constraint. I do pass mfpmath vfp to the test in case this gets fixed, but it's not strictly necessary right now.

The following is currently accepted:

int oops(int x) { // CHECK-LABEL: @oops(
  // CHECK: call float asm "vsqrt.f32 $0, $1", "=t,t"(float
  __asm__("vsqrt.f32 %0, %1"
          : "=t"(x)
          : "t"(x));
  return x;
}

Bitcode:

define arm_aapcscc i32 @oops(i32 %x) #1 {
  %1 = alloca i32, align 4
  store i32 %x, i32* %1, align 4
  %2 = load i32, i32* %1, align 4
  %3 = call i32 asm "vsqrt.f32 $0, $1", "=t,t"(i32 %2) #2, !srcloc !7
  store i32 %3, i32* %1, align 4
  %4 = load i32, i32* %1, align 4
  ret i32 %4
}

Here's a fun GCC comparison, which is also surprising in a different way.

Fixing this would be an entirely different patch.

jfb added a comment.Feb 18 2016, 6:11 PM

Is this ready to land? I'm hoping to get it into the 3.8 branch since it affects musl libc's latest release.

compnerd accepted this revision.Feb 18 2016, 7:59 PM
compnerd added a reviewer: compnerd.

Yeah, constraint validation is not one of the highlights of the current implementation. This seems reasonable enough to merge I think.

This revision is now accepted and ready to land.Feb 18 2016, 7:59 PM
This revision was automatically updated to reflect the committed changes.