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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for working on this. I left some comments and look forward to the updateImpl code :)



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:1706
+  static const char ID;
+};
+
----------------
In comments two words please "undefined behavior"

Remove the "AA" in the method names: "isKnownUndefinedBehavior" or "isKnownToCauseUB".

Return the state value not `true`/`false`. See other `isKnownXXXXX` functions around here.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1977
+      // TODO: Do something for every load instruction...
+      IRPosition IPos = IRPosition::callsite_function(ImmutableCallSite(&I));
+      return true;
----------------
I'm unsure what the IPos does here. Probably copy&paste. Note that `I` is known to be a `Load` here, so `ImmutableCallSite(&I)` will always result in a "unusable" call site, e.g., one that evaluates to `false` if converted to bool.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2001
+  void trackStatistics() const override {
+    STATS_DECLTRACK_CS_ATTR(undefinedbehavior)
+  }
----------------
We need to track statistics differently. Maybe we want to track two lists, one for assumed live and UB loads one for assumed live and non-UB loads. We can track the number of UB loads then by using something like this (sorry for the template mess)
```
STATS_DECL(UndefinedBehaviorInstruction, Instruction, "Number of instructions known to have UB");
BUILD_STAT_NAME(UndefinedBehaviorInstruction, Instruction) += AssumedUBInstructions.size();
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2003
+  }
+};
+
----------------
My proposal going forward:

Keep a list of loads that are *not* assumed to cause UB. Initially that list is empty.
Every updateImpl is called you iterate over all assumed live loads (as you do above) and:
  - skip ones we have in the list of *not* assumed UB causing
  - check if we can assume we can continue to assume execution would cause UB, initially by asking if the pointer is `null` or `undef`. If this fails add them to the list.

If the list changed the internal state changed and we have to tell the Attributor.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2012
+    AAUndefinedBehaviorImpl::initialize(A);
+  }
+
----------------
Let's not handle call sites for now and just give up on them.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5535
 
+  // Every function might be "undefined-behavior"
+  getOrCreateAAFor<AAUndefinedBehavior>(FPos);
----------------
The wording is a bit off. Maybe say something like: `Every function might contain instructions that cause "undefined-behavior"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71435