[PATCH] D18634: Don't IPO over functions that can be de-refined

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Comment changes suggested below.

This looks really great Sanjoy, and thanks for driving all of this. Feel free to commit with comment fixes.

Some additional things we really need here:

1. Send a heads up email to llvm-dev and cfe-dev. I think folks are going to be really, really surprised by regressions due to this and I don't want too much confusion.

2. Update the release notes with some of the context so that when the next release goes out we have documentation about why we're changing behavior here.


================
Comment at: include/llvm/IR/GlobalValue.h:103-124
@@ +102,24 @@
+  ///
+  /// These linkage types inhibits most non-inlining IPO, since a differently
+  /// optimized variant of the same function can have different observable or
+  /// undefined behavior than in the variant currently visible.  For instance,
+  /// we could have started with
+  ///
+  ///   void foo(int *v) {
+  ///     int t = 5 / v[0];
+  ///     (void) t;
+  ///   }
+  ///
+  /// and "refined" it to
+  ///
+  ///   void foo(int *v) { }
+  ///
+  /// However, we cannot infer readnone for `foo`, since that would justify
+  /// DSE'ing a store to `v[0]` across a call to `foo`, which can cause
+  /// undefined behavior if the linker replaces the actual call destination with
+  /// the unoptimized `foo`.
+  ///
+  /// Inlining is okay across these linkage types, since the currently visible
+  /// variant is *a* correct implementation of the original source function; it
+  /// just isn't the *only* correct implementation.
+  bool mayBeDerefined() const {
----------------
I think you should move this *awesome* explanation to the comment on isDefinitionExact, and just leave the first paragraph here. This is the comment optimization authors need to read when calling the predicates.

================
Comment at: include/llvm/IR/GlobalValue.h:132-133
@@ +131,4 @@
+
+    default:
+      return mayBeOverridden();
+    }
----------------
Same issue as with mayBeOverridden IMO -- we should either have the default be conservatively correct or fully enumerate the switch.

================
Comment at: include/llvm/IR/GlobalValue.h:335-336
@@ +334,4 @@
+
+  /// Return true if this global's definition can be substituted with an
+  /// *arbitrary* definition at link time.
+  bool isInterposable() const { return mayBeOverridden(); }
----------------
Add some (hopefully short) hints about what the implications are for transformations and analyses.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:72
@@ -71,4 +71,3 @@
 
-  // Definitions with weak linkage may be overridden at linktime with
-  // something that writes memory, so treat them like declarations.
-  if (F.isDeclaration() || F.mayBeOverridden()) {
+  // Non-exact function definitions (see GlobalValue::mayBeDerefined) may be
+  // overridden at linktime with something that writes memory, so treat them
----------------
Update this cross reference when you update the comments.

================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:73
@@ +72,3 @@
+  // Non-exact function definitions (see GlobalValue::mayBeDerefined) may be
+  // overridden at linktime with something that writes memory, so treat them
+  // like declarations.
----------------
"be overridden at linktime with something that writes memory" -> "not be selected at link time, and an alternative version that writes to memory may be selected"

Mostly trying to avoid making it seem like this only happens intentionally.


http://reviews.llvm.org/D18634