Page MenuHomePhabricator

Value soft float calls as more expensive in the inliner.
ClosedPublic

Authored by dirty on Jan 12 2015, 2:48 PM.

Details

Summary

When evaluating floating point instructions in the inliner, ask the TTI whether it is an expensive operation. By default, it's not an expensive operation. This keeps the default behavior the same as before. The ARM TTI has been updated to return back TCC_Expensive for targets which don't have hardware floating point.

Diff Detail

Event Timeline

dirty updated this revision to Diff 18049.Jan 12 2015, 2:48 PM
dirty retitled this revision from to Value soft float calls as more expensive in the inliner..
dirty updated this object.
dirty edited the test plan for this revision. (Show Details)
dirty added a subscriber: Unknown Object (MLST).
dirty added a comment.Jan 15 2015, 4:22 PM

Ping

Cameron Esfahani
dirty@apple.com

"Americans are very skilled at creating a custom meaning from something that's mass-produced."

Ann Powers

Hi Cameron,

We could probably do with some tests for this, and I think the ARM logic needs a bit of work.

lib/Target/ARM/ARMSubtarget.h
316–318

I don't think this is quite right. The relevant code in ARMISelLowering is:

if (!TM.Options.UseSoftFloat && Subtarget->hasVFP2() &&
    !Subtarget->isThumb1Only())

(notably: any CPU with float has VFP2; SoftFloat can be specified even if the hardware supports it; and Thumb1 didn't have access to VFP).

lib/Target/ARM/ARMTargetTransformInfo.cpp
318

I don't think this is quite right. The relevant code in ARMISelLowering is:

if (!TM.Options.UseSoftFloat && Subtarget->hasVFP2() &&

!Subtarget->isThumb1Only())

(notably: any CPU with float has VFP2; SoftFloat can be specified even if the hardware supports it; and Thumb1 didn't have access to VFP).

dirty added a comment.Jan 23 2015, 3:44 PM

I have a unit test for this patch, but it's in the clang side, so I will submit it after this patch gets approved.

I've changed the code to adopt your suggestions. I'll send out an updated patch soon.

Cameron Esfahani
dirty@apple.com

"I cannot for the life of me understand why, while people without driver's licenses are not allowed on public roads, in bookstores one can find any number of books by persons without decency - let alone knowledge."

"His Master's Voice", Stanislaw Lem

echristo edited edge metadata.Jan 23 2015, 4:25 PM

... I'm curious how the testcase is on the clang side.

-eric

dirty abandoned this revision.Jan 23 2015, 4:54 PM
dirty reclaimed this revision.
dirty added a comment.Jan 23 2015, 4:58 PM

It's in the CodeGen directory. Here's the current testcase:

[dirty@girlyouknowitstrue work]$ cat llvm/tools/clang/test/CodeGen/inline-fp.c
RUN: %clang -Oz -target armv7-apple-macosx10.8.0 -mcpu=cortex-a8 -march=armv7 -S -emit-llvm %s -o - | FileCheck %s
RUN: %clang -Oz -target armv7-apple-macosx10.8.0 -mcpu=cortex-m3 -march=armv7 -S -emit-llvm %s -o - | FileCheck -check-prefix NO-FP %s

extern float powf(float x, float y);
extern float fabsf(float x);

static float f1(int response, unsigned char value1) {

float value2 = 2620.0f * powf(1.01f, value1 - 1);
float responseDelta = (response - value2) / value2;

return (responseDelta);

}

extern void getX(int *responseX, unsigned char *valueX);
extern void getY(int *responseY, unsigned char *valueY);
extern void getZ(int *responseZ, unsigned char *valueZ);

CHECK-NOT: define internal arm_aapcscc float @f1(
NO-FP: define internal arm_aapcscc float @f1(
int test_all(void) {

int responseX;
int responseY;
int responseZ;
unsigned char valueX;
unsigned char valueY;
unsigned char valueZ;

getX(&responseX, &valueX);
getY(&responseY, &valueY);
getZ(&responseZ, &valueZ);

float responseDeltaX = f1(responseX, valueX);
float responseDeltaY = f1(responseY, valueY);
float responseDeltaZ = f1(responseZ, valueZ);

int success = 1;

if (fabsf(responseDeltaX) > 0.14f)
  success = 0;
else if (fabsf(responseDeltaY) > 0.14f)
  success = 0;
else if (fabsf(responseDeltaZ) > 0.14f)
  success = 0;

return (success);

}

Cameron Esfahani
dirty@apple.com

"Americans are very skilled at creating a custom meaning from something that's mass-produced."

Ann Powers

dirty updated this revision to Diff 18701.Jan 23 2015, 5:00 PM
dirty edited edge metadata.

Integrate patch suggestions from Tim Northover: use the same logic in ARMTTI as in ARMISelLowering.

Yeah, not so much. Testcases like that need to be in the backend. No
optimization testcases in the front end if we can help it. (i.e. testing
options)

Thanks!

-eric

dirty added a comment.Jan 23 2015, 5:12 PM

Okay, I'll move the test case. Thanks for the feedback.

dirty updated this revision to Diff 18793.Jan 26 2015, 6:08 PM

Update patch to account for routines which have the "use-soft-float" attribute. Add test case where we make sure that routines marked with "use-soft-float" are not inlined, but routines without that attribute are.

dirty updated this revision to Diff 19203.Feb 2 2015, 4:24 PM

Update patch to work with Chandler's changes to TTI. Removed reference to Options.UseSoftFloat, as it's eventually going away and function attribute "use-soft-float" is more appropriate.

echristo accepted this revision.Feb 3 2015, 2:49 PM
echristo edited edge metadata.

Can probably expand the testcase a bit more with CHECK/CHECK-NOT for the various calls? One inline comment.

Otherwise LGTM.

-eric

lib/Analysis/IPA/InlineCost.cpp
911

s/will/may.

This revision is now accepted and ready to land.Feb 3 2015, 2:49 PM
dirty updated this revision to Diff 19279.Feb 3 2015, 3:48 PM
dirty edited edge metadata.

Update test case with feedback from echristo.

dirty updated this revision to Diff 19283.Feb 3 2015, 4:12 PM

Clean up test a little more: use CHECK-LABEL and move tests to relevant routines.

LGTM. Can you commit or do you need someone to commit for you?

-eric

dirty closed this revision.Feb 4 2015, 6:11 PM