This is an archive of the discontinued LLVM Phabricator instance.

Don't Promote x86_fp80 byval Pointer Arguments
ClosedPublic

Authored by tjablin on Aug 25 2014, 5:58 PM.

Details

Summary

Don't promote byval pointer arguments when when their size in bits is not equal to their alloc size in bits. This can happen for x86_fp80, where the size in bits is 80 but the alloca size in bits in 128. Promoting these types can break passing unions of x86_fp80s and other types.

Diff Detail

Repository
rL LLVM

Event Timeline

tjablin updated this revision to Diff 12927.Aug 25 2014, 5:58 PM
tjablin retitled this revision from to Don't Promote x86_fp80 byval Pointer Arguments.
tjablin updated this object.
tjablin edited the test plan for this revision. (Show Details)
tjablin updated this object.
tjablin edited the test plan for this revision. (Show Details)
tjablin added a reviewer: rnk.
tjablin added subscribers: Unknown Object (MLST), rnk.
tjablin removed a subscriber: rnk.
rnk edited edge metadata.Aug 25 2014, 6:08 PM

It occurs to me that we can actually accomplish the promotion if all loads from the byval struct use the x86_fp80 type. We just need to make sure that nobody is loading from the other bits. I think the right fix is probably to add logic to the checking we do on the loads.

Okay. I'll rethink it tomorrow.

tjablin updated this revision to Diff 12932.Aug 25 2014, 6:52 PM
tjablin edited edge metadata.

If the members of a structure passed by value are used only through geps and loads, the isSafeToPromoteArgument function will handle them correctly, so contrary to its name, the patch will allow byval x86_fp80*s to be promoted, provided they are used in a sane way. I have added a new testcase where the argument is used sanely, and consequently its argument is promoted.

rnk added inline comments.Aug 25 2014, 8:16 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
191 ↗(On Diff #12932)

I think this code needs to handle this case as well:

%struct.Foo = type { i32, i64 }
define internal i64 @foo(%struct.Foo* byval %a) {
  %p = bitcast %struct.Foo* %a to i64*
  %v = load i64* %p
  ret i64 %v
}

In retrospect, the size comparison was probably correct. I just didn't realize that we fell back to the usual promotion checks that would allow us to promote the simple, castless cases.

test/Transforms/ArgumentPromotion/fp80.ll
18 ↗(On Diff #12932)

I recommend writing positive test cases when possible, so that the test case doesn't silently break. For example, if someone renamed @foo without fixing the CHECK we wouldn't know. That's contrived, but there are other cases. The negative grep tests in particular have proven to be not very useful. You can probably look for all of:

; CHECK: define internal fastcc void @foo(%union.u* byval
tjablin updated this revision to Diff 12953.Aug 26 2014, 10:21 AM

Hi Reid,

I have updated the test case to include your { i32, i64 } example from yesterday. The structure is still correctly promoted. I switched from the DL->getAllocSize approach to testing for the x86_fp80 type explicitly, since DL is not guaranteed to be available in the ArgumentPromotion pass.

Looking at the implementation of DataLayout, I can see that the alloc size only differs from the store size when the store size mod the ABI alignment is not zero. Other than x86_fp80, there's no way this can happen for C or C++ codes (where the size of an aggregate is guaranteed to be a multiple of its alignment).

Tom

Unless you think that the packing bits between the first and second members of the {i32, i64} structure should be preserved, in which case this is not correct.

rnk added a comment.Aug 26 2014, 12:41 PM
In D5057#14, @tjablin wrote:

Unless you think that the packing bits between the first and second members of the {i32, i64} structure should be preserved, in which case this is not correct.

If you remove byval and just pass a pointer, you can absolutely access those padding bits in LLVM. I don't think adding byval should change this. If you pass a first-class aggregate instead, then the semantics change, because the semantics of FCAs are the same as passing each element separately.

tjablin updated this revision to Diff 12998.Aug 27 2014, 1:46 PM

Hi Reid,
I have prepared a new version of the patch to address your feedback. The patch includes new tests, so arguments are only promoted if they have no padding or if we can prove the padding bytes are not accessed. In either case, it is safe to pass the elements by value without worrying about complications due to padding bytes. I have added a data layout field to the tail.ll test case. On a platform where i32 is 64-bit aligned, the promotion tested by tail.ll is unsound.
Tom

rnk added a comment.Aug 27 2014, 2:09 PM

Almost there, I think.

lib/Transforms/IPO/ArgumentPromotion.cpp
132 ↗(On Diff #12998)

David, is there an easier way to write this function in LLVM? You are more familiar with writing optimizations.

175 ↗(On Diff #12998)

This seems like an expensive duplication of the work you do below.

186–187 ↗(On Diff #12998)

You need to check that V is the target of the load or store, and not the value being loaded or stored. Adding this check should subsume PointerMayBeCaptured. We should have a test case where we store the address of a byval parameter to a global, for example.

tjablin updated this revision to Diff 13011.Aug 27 2014, 4:04 PM

Hi Reid,
I have updated the patch to remove the dependence on PointerMayBeCaptured, and also to add support for searching through PHINodes. Per your suggestion, I have added a new test case to verify that captured values will not be promoted.
Tom

rnk added a comment.Aug 28 2014, 10:12 AM

Last round of nits, I promise.

lib/Transforms/IPO/ArgumentPromotion.cpp
130 ↗(On Diff #13011)

This function changed names. I think these days we just use \brief, @brief, or nothing for doxygen comments.

130 ↗(On Diff #13011)

This sentence and others should end in a full stop.

165 ↗(On Diff #13011)

ditto

test/Transforms/ArgumentPromotion/fp80.ll
39 ↗(On Diff #13011)

Can we check for the whole prototype? I want to know if we remove byval in this case.

tjablin updated this revision to Diff 13049.Aug 28 2014, 11:45 AM

As per request, I have modified the comments to use \brief, added full-stops at the ends of sentences, and modified the unit tests to search for whole prototypes. Please let me know if any additional modifications are necessary. Otherwise, could you please push this upstream?

tjablin updated this revision to Diff 13050.Aug 28 2014, 11:54 AM

Add more full-stops.

rnk closed this revision.Aug 28 2014, 3:51 PM
rnk updated this revision to Diff 13061.

Closed by commit rL216693 (authored by @rnk).