[PATCH] D71799: [Attributor] AAUndefinedBehavior: Check for branches on undef value.

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked an inline comment as done.
baziotis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2141
 private:
-  // A set of all the (live) memory accessing instructions
-  // that are _not_ assumed to cause UB.
+  // A set of all the (live) instructions that are known to _not_ cause UB.
   //   Note: The correctness of the procedure depends on the fact that this
----------------
jdoerfert wrote:
> baziotis wrote:
> > baziotis wrote:
> > > uenoku wrote:
> > > > Could you add a comment here to say that instruction in `NoUBInst` might cause UB?
> > > Of course.
> > I was thinking about it and it seems to me that having this set is kind of misleading (both the naming and the conceptual idea around it).
> > That is, it doesn't seem that there's a way (at least for now) to know for sure that an instruction is not UB, unless we have a constant.
> > So, I'm proposing to change the conceptual idea (and the naming) like this. An instruction can be in 3 categories:
> > 1) Known to cause UB (`AAUndefinedBehavior` could prove it) - make an actual set for it.
> > 2) Known to not cause UB (`AAUndefinedBehavior` could prove it because of a constant). Here would go only branch instructions, which could have a constant condition and not be UB. Memory accessing instructions can't AFAIK because if they have a constant, it will either be null (so UB) or undef (so, UB). Make another set for those (so that we don't re-process them in every update)
> > 3) Assumed to cause UB. Basically, every other instruction.
> > 
> > What we have now is sort of this scheme, but the `KnownNoUBInsts` are not actually known to not be UB as you mentioned and hence I think this makes the understanding of the code difficult.
> > 
> > (For reference, and you may as well skip that since this comment is already big, I think the current scheme is something like:
> > An instruction can be:
> > 1) Known to cause UB (`AAUndefinedBehavior` could prove it).
> > 2) Assumed to not cause UB. `AAUndefinedBehavior` could _not_ prove it but still it optimistically assumes it doesn't cause UB
> > (which is like "what??" since `AAUndefinedBehavior` is supposed to optimistically assume for UB).
> > ...)
> > Anyway, looking forward to your opinion and sorry for the big comment (on an already accepted revision).
> The two interesting categories are:
> 
> 1) Known to cause UB (and we proved it)
> 2) Assumed to cause UB (every updateImpl invocation we found a reason to assume it. This is probably not what this Attribute does but it should eventually).
> 
> The third category which is tracked so we don't revisit instructions that do not fall into the first two is:
> 
> 3) Not assumed to cause UB. We failed to argue it causes UB in an updateImpl invocation. This includes things that cannot cause UB! (We track things that do cause UB not the other way around.)
Ok, got it, thanks! FWIW, I didn't propose the "known no UB" set because I think it is a good idea to track "no UB" instructions.
Rather, that schemeis overoptimistic because for every instruction that we could not //prove// it is no UB, it assumes it is UB.
Yours is clearly better. Meaning we should have //a reason// to assume an instruction is UB.
I'll update it asap.


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

https://reviews.llvm.org/D71799