[PATCH] D83952: [SCCP] Add range metadata to call sites with known return ranges.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/Transforms/SCCP/ip-add-range-to-call.ll:28
 ; CHECK-LABEL: @caller2(
 ; CHECK-NEXT:    [[X_15:%.*]] = and i32 [[X:%.*]], 15
+; CHECK-NEXT:    [[C:%.*]] = call i32 @callee(i32 [[X_15]]), !range [[RANGE_10_21]]
----------------
nikic wrote:
> fhahn wrote:
> > efriedma wrote:
> > > What does it mean to say the range doesn't "include undef", here?
> > > 
> > > If the argument is poison, does this introduce UB?
> > > What does it mean to say the range doesn't "include undef", here?
> > 
> > It is meant to refer to the 'range including undef' notion in ValueLattice. So it means that we know the result value cannot be undef, because all values merged with the return value state are known to not be undef. Reading it back it seems on its own it is not entirely clear. Please let me know if that makes sense, then I'll update the comment.
> > 
> > > If the argument is poison, does this introduce UB?
> > 
> > Hm, thinking a bit more about it, I guess it could.
> > 
> >  So for this example function, if `%x` would be poison, the result of the AND would also be poison, but we would add the range metadata nonetheless.
> > But returning a value outside of the !range is UB. So I guess we introduced extra UB because poison is outside the range. Was that the case you had in mind? 
> > 
> > It seems like the fact that returning values outside of !range is immediate UB makes it quite tricky to synthesize !range in the presence of of poison generally.
> > 
> > I suppose we have 2 options here:
> > 1. Check that the return value is guaranteed to not be poison before adding the range.
> > 2. Adjusting the definition of !range for calls to make returning poison (= always outside the range) lead to poison, rather than immediate UB or making returning values outside the range poison directly instead.
> > 
> > Doing 1. in the patch should be fairly straight forward, but 2. might be more beneficial in the long term. What do you think?
> > Adjusting the definition of !range for calls to make returning poison (= always outside the range) lead to poison, rather than immediate UB or making returning values outside the range poison directly instead.
> 
> This is the direction things are moving for call attributes, though I don't think any patches are up yet. Things like nonnull will only yield poison, and immediate UB will be introduced (if desired) by the noundef attribute. Keeping that in mind, it would make sense if !range and !nonnull followed the same behavior.
> This is the direction things are moving for call attributes, though I don't think any patches are up yet. Things like nonnull will only yield poison, and immediate UB will be introduced (if desired) by the noundef attribute. Keeping that in mind, it would make sense if !range and !nonnull followed the same behavior.

Hm, having direct UB seems more convenient for the consumers of the metadata, as it rules out the result being poison/undef, allowing the information to be used directly. If we relax it to poison, things might get more difficult for consumers. But then, for callsites there's the noundef attribute to restore old behavior. That leaves loads, for which there would be no way to restore immediate UB I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83952