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

[• koriakin]

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.

[davidxl]

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

[• koriakin]

Changed to live in TTI.

[dsanders]

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]

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]

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]

Added another hook for mips.

[davidxl]

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.

[• koriakin]

Split off the target-specific parts.

[davidxl]

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

[hfinkel]

In D21739#469090, @davidxl wrote:

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.

[• koriakin]

In D21739#469548, @hfinkel wrote:

In D21739#469090, @davidxl wrote:

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]

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

[• koriakin]

Ping?

[• koriakin]

Ping?

[• koriakin]

@hfinkel Ping?

[hfinkel]

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]

Hi Marcin,

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

Thanks,
Simon

[• koriakin]

In D21739#562041, @sdardis wrote:

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.

[• koriakin]

Moved the functions to TLI, as per ML: http://lists.llvm.org/pipermail/llvm-dev/2016-September/104641.html.

[• koriakin]

Ping?

[davidxl]

Is related TLI change upstreamed?

[• koriakin]

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

[davidxl]

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

[hfinkel]

In D21739#589647, @davidxl wrote:

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.

[efriedma]

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?

[• koriakin]

In D21739#587028, @davidxl wrote:

Is related TLI change upstreamed?

In D21739#589870, @hfinkel wrote:

In D21739#589647, @davidxl wrote:

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]

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]

lib/Analysis/TargetLibraryInfo.cpp

426

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

[hfinkel]

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)

[• koriakin]

Changed to composite getExtAttrForI32Param/Return functions.

lib/Analysis/TargetLibraryInfo.cpp

426

Sounds good, done.

[hfinkel]

LGTM

[alekseyshl]

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.

[• koriakin]

In D21739#601531, @alekseyshl wrote:

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.

include/llvm/Analysis/TargetLibraryInfo.h

 /// depends on the triple. So users typically interact with the \c
 /// TargetLibraryInfo wrapper below.
 class TargetLibraryInfoImpl {
   friend class TargetLibraryInfo;
 
   unsigned char AvailableArray[(LibFunc::NumLibFuncs+3)/4];
   llvm::DenseMap<unsigned, std::string> CustomNames;
   static const char *const StandardNames[LibFunc::NumLibFuncs];
+  bool ShouldExtI32Param, ShouldExtI32Return;
 
   enum AvailabilityState {
     StandardName = 3, // (memset to all ones)
     CustomName = 1,
     Unavailable = 0  // (memset to all zeros)
   };
   void setState(LibFunc::Func F, AvailabilityState State) {
     AvailableArray[F/4] &= ~(3 << 2*(F&3));
bool isFunctionScalarizable(StringRef F, unsigned &VF) const {
     return !getScalarizedFunction(F, VF).empty();
   }
 
   /// Return the name of the equivalent of F, scalarized. If no such mapping
   /// exists, return the empty string.
   ///
   /// Set VF to the vectorization factor.
   StringRef getScalarizedFunction(StringRef F, unsigned &VF) const;
+
+  /// Set whether int and unsigned int parameters should have
+  /// signext or zeroext attributes, respectively.
+  void setShouldExtI32Param(bool Param) {
+    ShouldExtI32Param = Param;
+  }
+
+  /// Set whether int and unsigned int returns should have
+  /// signext or zeroext attributes, respectively.
+  void setShouldExtI32Return(bool Return) {
+    ShouldExtI32Return = Return;
+  }
 };
 
 /// Provides information about what library functions are available for
 /// the current target.
 ///
 /// This both allows optimizations to handle them specially and frontends to
 /// disable such optimizations through -fno-builtin etc.
 class TargetLibraryInfo {
StringRef getName(LibFunc::Func F) const {
     if (State == TargetLibraryInfoImpl::Unavailable)
       return StringRef();
     if (State == TargetLibraryInfoImpl::StandardName)
       return Impl->StandardNames[F];
     assert(State == TargetLibraryInfoImpl::CustomName);
     return Impl->CustomNames.find(F)->second;
   }
 
+  /// Returns true iff i32 parameters to library functions should have
+  /// signext or zeroext attributes if they correspond to C-level int or
+  /// unsigned int, respectively.
+  bool shouldExtI32Param() const {
+    return Impl->ShouldExtI32Param;
+  }
+
+  /// Returns true iff i32 returns from library functions should have
+  /// signext or zeroext attributes if they correspond to C-level int or
+  /// unsigned int, respectively.
+  bool shouldExtI32Return() const {
+    return Impl->ShouldExtI32Return;
+  }
+
   /// Handle invalidation from the pass manager.
   ///
   /// If we try to invalidate this info, just return false. It cannot become
   /// invalid even if the module changes.
   bool invalidate(Module &, const PreservedAnalyses &) { return false; }
 };
 
 /// Analysis pass providing the \c TargetLibraryInfo.
 ///
 /// Note that this pass's result cannot be invalidated, it is immutable for the
 /// life of the module.

lib/Analysis/TargetLibraryInfo.cpp

static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
   if (T.isNVPTX()) {
     TLI.disableAllFunctions();
     TLI.setAvailable(LibFunc::nvvm_reflect);
   } else {
     TLI.setUnavailable(LibFunc::nvvm_reflect);
   }
 
   TLI.addVectorizableFunctionsFromVecLib(ClVectorLibrary);
+
+  // PowerPC64, Sparc64, SystemZ need signext/zeroext on i32 parameters and
+  // returns corresponding to C-level ints.  Mips64 needs it for parameters
+  // only.
+  bool ShouldExtI32Param = false, ShouldExtI32Return = false;
+  if (T.getArch() == Triple::ppc64 || T.getArch() == Triple::ppc64le ||
+      T.getArch() == Triple::sparcv9 || T.getArch() == Triple::systemz) {
+    ShouldExtI32Param = true;
+    ShouldExtI32Return = true;
+  }
+  if (T.getArch() == Triple::mips64 || T.getArch() == Triple::mips64el)
+    ShouldExtI32Param = true;
+  TLI.setShouldExtI32Param(ShouldExtI32Param);
+  TLI.setShouldExtI32Return(ShouldExtI32Return);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl() {
   // Default to everything being available.
   memset(AvailableArray, -1, sizeof(AvailableArray));
 
   initialize(*this, Triple(), StandardNames);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(const Triple &T) {
   // Default to everything being available.
   memset(AvailableArray, -1, sizeof(AvailableArray));
 
   initialize(*this, T, StandardNames);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(const TargetLibraryInfoImpl &TLI)
-    : CustomNames(TLI.CustomNames) {
+    : CustomNames(TLI.CustomNames), ShouldExtI32Param(TLI.ShouldExtI32Param),
+      ShouldExtI32Return(TLI.ShouldExtI32Return) {
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
   VectorDescs = TLI.VectorDescs;
   ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(TargetLibraryInfoImpl &&TLI)
-    : CustomNames(std::move(TLI.CustomNames)) {
+    : CustomNames(std::move(TLI.CustomNames)),
+      ShouldExtI32Param(TLI.ShouldExtI32Param),
+      ShouldExtI32Return(TLI.ShouldExtI32Return) {
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
             AvailableArray);
   VectorDescs = TLI.VectorDescs;
   ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(const TargetLibraryInfoImpl &TLI) {
   CustomNames = TLI.CustomNames;
+  ShouldExtI32Param = TLI.ShouldExtI32Param;
+  ShouldExtI32Return = TLI.ShouldExtI32Return;
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
   return *this;
 }
 
 TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(TargetLibraryInfoImpl &&TLI) {
   CustomNames = std::move(TLI.CustomNames);
+  ShouldExtI32Param = TLI.ShouldExtI32Param;
+  ShouldExtI32Return = TLI.ShouldExtI32Return;
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
             AvailableArray);
   return *this;
 }
 
 static StringRef sanitizeFunctionName(StringRef funcName) {
   // Filter out empty names and names containing null bytes, those can't be in
   // our table.