[PATCH] D85632: [Attributor] Implement AAPotentialValues

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7343
+                                       const APInt &LHS, const APInt &RHS,
+                                       bool &Valid) {
+    Instruction::BinaryOps BinOpcode = BinOp->getOpcode();
----------------
okura wrote:
> jdoerfert wrote:
> > Add a TODO that we should look at `nsw` and `nuw` to help, e.g., to exclude certain results that cannot happen without creating `undef`. Similarly, division by `undef` should not make the result invalid but simply skip this operand pair.
> I understand you mean that we can assume the operation result is not a poison value or undef and that we can ignore such operand pair. Is that right?
> I'm thinking of adding a new argument `bool &Unsupported` to this to distinguish the above case from the case opcode is unsupported. 
> In the former case we skip the operand pair, and in the latter case we give up (i.e. call `indicatePessimisticFixpoint`).
Yes and no. So there are two things.
1) If the operation would be poison or undef, we can add undef to the set instead of something else. Undef is strictly better than any other APInt. This is the case for the `nuw`/`nsw` binary ops but I would not go there right now. A TODO makes more sense and a follow up patch.
2) If the operation would be UB, that is it can actually not happen, we can ignore the result. Think of a division in which the RHS is {0, 1}. Since division by 0 is UB, we can ignore that result and only take the 1 result.

Have such a bool reference argument makes sense, maybe call it "SkipOperation" or something and pass it through to the caller so we can just go ahead and ignore this combination of LHS/RHS.


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

https://reviews.llvm.org/D85632