[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]]
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83952