[PATCH] D71435: [WIP] [Attributor] Function level undefined behavior attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Almost there. Once a manifest method (see below) is there to act on the information we should be able to test it. So in addition to the inlined comments we need test cases now.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1699
+  /// Return true if "undefined behavior" is known.
+  bool isKnownToCauseUB() const { return getKnown(); }
+
----------------
Since we start with a function version only, we probably need to add an `llvm::Instruction` operand here.
While it makes sense if *every* execution of a function causes UB, the initial implementation will
answer the questions for a single (load) instruction.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1995
+
+  SmallPtrSet<Instruction *, 8> NoUBLoads;
+
----------------
Please document what is collected in here. Maybe make it not accessible for other classes, e.g., move it to the end into a `private:` part of the struct declaration.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2023
+          if (!NullPointerIsDefined)
+            CanCauseUB = true;
+        }
----------------
You can assume `I` to be in a function, thus `I.getFunction()` is not null.

If you write the code with early exists you can save multiple levels of nesting and make it generally easier to read.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2040
+  }
+};
+
----------------
Let's also add a manifest method to replace the loads that are still considered UB with an undef (for now). That should allow us to test this.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2051
+    BUILD_STAT_NAME(UndefinedBehaviorInstruction, Instruction) +=
+        NoUBLoads.size();
+  }
----------------
This was my bad, as I proposed this kind of statistic tracking, but this counts the opposite of what the text says. You can keep a different set with the ones that are still considered UB which you update in the updateImpl as well. That set can also be used in the manifest method.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71435/new/

https://reviews.llvm.org/D71435