This is an archive of the discontinued LLVM Phabricator instance.

[TLI] Add functions determining if int parameters/returns should be zeroext/signext.
ClosedPublic

Authored by koriakin on Jun 26 2016, 4:24 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. Add this
information to TargetLibraryInfo, to be used whenever some LLVM pass
inserts a compiler-rt call to a function involving int parameters
or returns.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [TLI] Add functions determining if int parameters/returns should be zeroext/signext..
koriakin updated this object.
koriakin added reviewers: vsk, davidxl.
koriakin set the repository for this revision to rL LLVM.
koriakin added a subscriber: llvm-commits.
davidxl edited edge metadata.Jun 27 2016, 10:43 AM

This looks like the wrong place for the interface. TargetTransformInfo (TTI) is better suited.

koriakin retitled this revision from [TLI] Add functions determining if int parameters/returns should be zeroext/signext. to [TTI] Add functions determining if int parameters/returns should be zeroext/signext..
koriakin updated this object.
koriakin edited edge metadata.

Changed to live in TTI.

dsanders added inline comments.Jun 28 2016, 2:56 AM
lib/Target/Mips/MipsTargetTransformInfo.h
42 ↗(On Diff #62042)

This doesn't look correct to me. On our target we unconditionally add the signext attribute to i32 in the frontend. This only has a significant effect for the 64-bit ABI's (N32 and N64) but it's still useful for the 32-bit ABI's (O32) as a reminder of how 32-bit code operates when run on a 64-bit processor (every instruction from the 32-bit subset sign-extends the result value from 32-bit to register width).

Returns also behave the same way as parameters.

koriakin added inline comments.Jun 28 2016, 3:05 AM
lib/Target/Mips/MipsTargetTransformInfo.h
42 ↗(On Diff #62042)

So, for Mips, we'd need a third predicate that forces a signext even for unsigned parameters, right?

Returns don't seem to get any ext attribute, though:

unsigned f(unsigned x) {

return x;

}

gets compiled by clang to:

define i32 @f(i32 signext) #0 { ... }

dsanders added inline comments.Jun 28 2016, 4:07 AM
lib/Target/Mips/MipsTargetTransformInfo.h
42 ↗(On Diff #62042)

So, for Mips, we'd need a third predicate that forces a signext even for unsigned parameters, right?

That's right except it's just 'unsigned int' that ignores the signedness rather than all unsigned parameters. The other unsigned integers use a sign-dependant extension. For example, unsigned short gets zeroext because it's equivalent to the two-step extension (zero-extend to i32, sign-extend to register width) described by our calling convention.

Returns don't seem to get any ext attribute, though:

unsigned f(unsigned x) {

return x;
}

gets compiled by clang to:

define i32 @f(i32 signext) #0 { ... }

That's worrying, there's probably a subtle sign/zero-extension bug lurking somewhere. It's often difficult to identify them since CPU's tend to be more lenient than our ISA (e.g. a 32-bit addition using register values 0x1_80000000 and 0x0 is 'unpredictable' according to our ISA but most implementations will give the expected 0xffffffff_80000000).

I'll mention this likely bug to the people working on the sanitizers since they're tracking down a sign/zero extension bug on MIPS64 at the moment.

koriakin updated this object.

Added another hook for mips.

Target specific implementation of the patch needs target owners to review. For this reason, I think it is better to to this

  1. make the interface change with default implementation a separate patch as NFC. After that is checked in, your InstrProfile patches can be unblocked and checked in (even though s390 target is still broken)
  2. split out one patch per affected target and add owners to review.

Split off the target-specific parts.

Looks reasonable to me. Added Hal for a second look.

hfinkel edited edge metadata.Jun 28 2016, 6:53 PM

Looks reasonable to me. Added Hal for a second look.

If a given target is not compiled in, then the defaults TTI provides might be wrong for the target. I think you need to add some what for the transformation to detect whether or not it is getting an accurate answer so it can abort the transformation in that case. We shouldn't miscompile IR just because we don't have a particular target available.

Looks reasonable to me. Added Hal for a second look.

If a given target is not compiled in, then the defaults TTI provides might be wrong for the target. I think you need to add some what for the transformation to detect whether or not it is getting an accurate answer so it can abort the transformation in that case. We shouldn't miscompile IR just because we don't have a particular target available.

Right. I will abort the InstrProfiling pass (and sanitizer passes that need this information) if the target is unknown. There are also a few optimizations that construct library calls, but I suppose these can be safely skipped if TTI says the target is unrecognized.

I'm going to add another function to TTI, isTargetRecognized, which will return true iff the target has been found in the registry (and thus got an opportunity to supply a custom TTI, even if it didn't use it). The default implementations of shouldExt* will assert isTargetRecognized(), and passes will need to check for it before calling shouldExt* functions (and perhaps other future queries which have to return correct information), failing or skipping optimization otherwise. Does that sound good?

koriakin edited edge metadata.

Added knownTarget() hook - it returns true if the TTI was created for a specific target, false if it was created for an unknown target.

All of our TTI interfaces so far have a safe/conservative default. This would change that, and I don't think that's a good idea. I'm also pretty undecided on whether this should live in TTI at all. If it does live in TTI, I think that the functions should return Optional<bool> , or some other tristate value (yes, no, unknown), with the default being unknown.

While, in general, I'm in favor of moving target information into the targets; this is a big step because it is not a cost-model issue, but a correctness issue. The information in question currently lives in the frontends, and while I find that unfortunate, I'm not sure that the ability to insert runtime-library functions, at the IR level, should be restricted to compiled-in targets. We might just want to put this information in LLVM itself (even though it would be target-specific, we do have some of that regardless - we even have target-specific data types).

Or maybe this information should be added as function attributes by the frontend?

I think it would be best to post an RFC on this to llvm-dev. I don't feel comfortable approving a direction here without community awareness.

include/llvm/Analysis/TargetTransformInfo.h
601 ↗(On Diff #62605)

Saying "library function" here is bound to be ambiguous. Do you mean functions with a certain calling convention? Do you mean only "internal" runtime-library functions?

616 ↗(On Diff #62605)

I don't like using the term "trusted" here. Also, a known target still might not implement all interfaces (and no target implements all interfaces optimally). I suspect this interface design will prove dangerous.

include/llvm/Analysis/TargetTransformInfoImpl.h
375 ↗(On Diff #62605)

If these are defaults that are never intended to be called, then they should assert. Please have these call llvm_unreachable instead off returning.

(However, please see my general comment on the interface)

sdardis added a subscriber: sdardis.Oct 5 2016, 4:05 AM

Hi Marcin,

This work got mentioned in an internal bug report, do you have any intention of reviving this patch series?

Thanks,
Simon

Hi Marcin,

This work got mentioned in an internal bug report, do you have any intention of reviving this patch series?

Thanks,
Simon

Yes, I'm going to revive this soon.

dsanders removed a subscriber: dsanders.Oct 5 2016, 6:35 AM
koriakin retitled this revision from [TTI] Add functions determining if int parameters/returns should be zeroext/signext. to [TLI] Add functions determining if int parameters/returns should be zeroext/signext..
koriakin updated this object.

Is related TLI change upstreamed?

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

This seems fine. Hal, do you have other concerns?

This seems fine. Hal, do you have other concerns?

Generally, I think this is the right approach. Do we only have a use case for i32, or would it apply to i16, i8, i1 too? If we only have a use case for i32 right now, I'm fine with leaving generalization to when we have use cases for other types.

Is the diff now missing something? I don't see anything here that would cause a functional change, but there's a test case which implies otherwise.

Exposing these methods with boolean return values directly to users seems a bit awkward... these checks will end up scattered all over the codebase. Maybe some sort of "computeAttributesForIntArgument" utility would be better?

Is related TLI change upstreamed?

This seems fine. Hal, do you have other concerns?

Generally, I think this is the right approach. Do we only have a use case for i32, or would it apply to i16, i8, i1 too? If we only have a use case for i32 right now, I'm fine with leaving generalization to when we have use cases for other types.

As far as I'm aware, only i32 need this treatment - i8 and i16 corresponding to C types are always signext/zeroext due to C standard working that way, and i64 tend to fill the whole register.

Is the diff now missing something? I don't see anything here that would cause a functional change, but there's a test case which implies otherwise.

The test was supposed to be attached to D21736, not this diff - fixed now.

hfinkel added inline comments.Nov 10 2016, 8:54 AM
include/llvm/Analysis/TargetLibraryInfo.h
299

I don't like this interface. Looking at D21736, for signed parameters, you need to call two query functions. Can you please combine these into one function like this:

bool shouldExtI32Param(bool Signed = true) const { ... }

Another option is to make two functions, like:

bool shouldExtSignedI32Param() const { ... }
bool shouldExtUnsignedI32Param() const { ... }

I don't have a strong preference.

lib/Analysis/TargetLibraryInfo.cpp
426

To be clear, they sign extend even unsigned numbers?

sdardis added inline comments.Nov 10 2016, 11:11 AM
lib/Analysis/TargetLibraryInfo.cpp
426

Yes. The N64 and N32 ABIs requires that 32 bit integer parameters are sign extended, even unsigned numbers.

hfinkel added inline comments.Nov 10 2016, 11:30 AM
lib/Analysis/TargetLibraryInfo.cpp
426

Ah, okay. In that case, I'll change my suggested interface to be something more direct, how about...

Attribute::AttrKind getExtAttrForI32Param(bool Signed = true) const { ... }

and then the user code can look like this:

if (auto AK = TLI->getExtAttrForI32Param())
  F->addAttribute(n, AK)

Changed to composite getExtAttrForI32Param/Return functions.

lib/Analysis/TargetLibraryInfo.cpp
426

Sounds good, done.

hfinkel accepted this revision.Nov 20 2016, 5:28 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 20 2016, 5:28 PM
This revision was automatically updated to reflect the committed changes.

It seems like this revision causes failures on sanitizer-x86_64-linux-bootstrap, at least check-clang ubsan is unhappy:

/mnt/b/sanitizer-buildbot2/sanitizer-x86_64-linux-bootstrap/build/llvm/lib/Analysis/TargetLibraryInfo.cpp:453:59: runtime error: load of value 32, which is not a valid value for type 'bool'

The actual loaded value varies.

It seems like this revision causes failures on sanitizer-x86_64-linux-bootstrap, at least check-clang ubsan is unhappy:

/mnt/b/sanitizer-buildbot2/sanitizer-x86_64-linux-bootstrap/build/llvm/lib/Analysis/TargetLibraryInfo.cpp:453:59: runtime error: load of value 32, which is not a valid value for type 'bool'

The actual loaded value varies.

Right, my bad - I didn't notice the initialize() function has an early return for AMDGPU targets and thus initialization of the new fields may not be reached. I'll just move this code to the beginning of the function, it should fix the failures.