This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] Mark __llvm_profile_instrument_target last parameter as i32 zeroext or signext if appropriate.
ClosedPublic

Authored by koriakin on Jun 26 2016, 2:10 PM.

Details

Summary

On some architectures (s390x, ppc64, sparc64, mips), C-level int is passed
as i32 signext instead of plain i32. Likewise, unsigned int may be passed
as i32, i32 signext, or i32 zeroext depending on the platform. Mark
__llvm_profile_instrument_target properly (its last parameter is unsigned
int).

This (together with the clang change) makes compiler-rt profile testsuite pass
on s390x.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [InstrProfiling] Mark __llvm_profile_instrument_target last parameter as i32 zeroext if appropriate..
koriakin updated this object.
koriakin added a reviewer: bogner.
koriakin set the repository for this revision to rL LLVM.
koriakin added a subscriber: llvm-commits.
vsk added subscribers: davidxl, vsk.Jun 26 2016, 3:33 PM

@davidxl would be a good reviewer for this.

include/llvm/Analysis/TargetLibraryInfo.h
178 ↗(On Diff #61913)

Could you split this up into two setters, to better match the getters below?

include/llvm/Transforms/InstrProfiling.h
35

It appears that TLI is assumed to be non-null. Can you make this a reference?

lib/Transforms/Instrumentation/InstrProfiling.cpp
177–178

Unrelated to this patch, but: it seems weird that this is called every time we do a lowerValueProfileInst(). Can we just do it once per run?

vsk added a comment.Jun 26 2016, 4:01 PM

Also, this is missing a test.

In D21736#467527, @vsk wrote:

Also, this is missing a test.

True. I'm going to add a clang test in a separate patch (this way it will cover D21737 as well). Should I make an llvm test as well?

vsk added a comment.Jun 26 2016, 4:10 PM
In D21736#467527, @vsk wrote:

Also, this is missing a test.

True. I'm going to add a clang test in a separate patch (this way it will cover D21737 as well). Should I make an llvm test as well?

Yes, that would be helpful.

The TLI change is independent of InstProfile changes. Please split the InstProf change into a separate patch.

koriakin updated this object.

Split off the TLI part, changed pointer to reference.

koriakin marked 2 inline comments as done.Jun 26 2016, 4:28 PM

Added a test.

vsk accepted this revision.Jun 27 2016, 8:23 AM
vsk added a reviewer: vsk.

LGTM.

This revision is now accepted and ready to land.Jun 27 2016, 8:23 AM

One nit. Can you save TLI pointer in InstProfiling instead of passing it around?

One nit. Can you save TLI pointer in InstProfiling instead of passing it around?

Save where? I thought run is not supposed to modify the Pass object, and I don't have TLI before that.

koriakin edited edge metadata.

Saved TLI as a field of InstProfiling.

davidxl accepted this revision.Jun 27 2016, 10:24 AM
davidxl added a reviewer: davidxl.

lgtm

koriakin updated this object.
koriakin edited edge metadata.

Changed to use TargetTransformInfo, per comment on D21739. D21737 is now no longer necessary.

I'm not sure if keeping TTI on the pass object is a good idea now...

koriakin retitled this revision from [InstrProfiling] Mark __llvm_profile_instrument_target last parameter as i32 zeroext if appropriate. to [InstrProfiling] Mark __llvm_profile_instrument_target last parameter as i32 zeroext or signext if appropriate..
koriakin updated this object.

Added another case for mips, which uses i32 signext.

koriakin updated this object.

Split off the tests (they'll come together with target-specific implementations for D21739).

Updated for new revision of D21739 - now aborts if knownTarget() is false.

koriakin removed rL LLVM as the repository for this revision.

It seems I have accidentally swapped the diffs of this revision and D21739. Here comes the correct diff.

koriakin set the repository for this revision to rL LLVM.

Readded missing test.

Changed to composite getExtAttrForI32Param/Return functions as for D21739.

hfinkel accepted this revision.Nov 20 2016, 5:34 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

LGTM too.

This revision was automatically updated to reflect the committed changes.