[PATCH] D21739: [TTI] Add functions determining if int parameters/returns should be zeroext/signext.

[Hal Finkel via llvm-commits]

hfinkel added a comment.

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.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:601
@@ -600,1 +600,3 @@
 
+  /// Returns true iff i32 parameters to library functions should have
+  /// signext or zeroext attributes if they correspond to C-level int or
----------------
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?

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:616
@@ +615,3 @@
+  /// Returns true iff the TTI was provided by a target (and thus the returned
+  /// information can be trusted), false if it's the default TTI constructed
+  /// for an unknown target.
----------------
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.

================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:375
@@ +374,3 @@
+
+  bool shouldExtI32Param() const { return false; }
+
----------------
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)


Repository:
  rL LLVM

https://reviews.llvm.org/D21739