[PATCH] D30184: [IR] Add a Instruction::dropPoisonGeneratingFlags helper

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D30184#681956, @majnemer wrote:

> Out of curiosity, why not use `clearSubclassOptionalData` in place of wherever you intend to use `dropPoisonGeneratingFlags`?


That seems like a layering problem -- there is no guarantee (is there?) that the bits in `SubclassOptionalData` correspond to poison.  We could conceivably add bits there in the future that (say) denote some probabilistic information (the result of this `add` is probably positive).  That would satisfy the contract of `SubclassOptionalData`, but would not require to be dropped in `dropPoisonGeneratingFlags`.



================
Comment at: lib/IR/Instruction.cpp:143
+  case Instruction::GetElementPtr:
+    cast<GetElementPtrInst>(this)->setIsInBounds(false);
+    break;
----------------
majnemer wrote:
> sanjoy wrote:
> > majnemer wrote:
> > > What about `inrange`?
> > Reading the language ref, `inrange` does not fit into the poison mould very well.
> > 
> > Say you have
> > 
> > ```
> > struct S {
> >   struct {
> >     int arr[2];
> >   } t0;
> >   struct {
> >     int arr[2];
> >   } t1;
> > };
> > ```
> > 
> > which in IR would be
> > 
> > ```
> > %struct.S = type { %struct.anon, %struct.anon.0 }
> > %struct.anon = type { [2 x i32] }
> > %struct.anon.0 = type { [2 x i32] }
> > 
> > ```
> > 
> > if I'm reading the ref correctly (am I?), then in
> > 
> > ```
> > %a = getelementptr %struct.S, %struct.S* %0, i64 0, inrange i32 0, i32 0, i64 0
> > %b = getelementptr i32, i32* %a, i32 2
> > %c = load i32, i32* %b
> > ```
> > 
> > the load has undefined behavior.  In the poison view of things, we'd like to say that either `%a` or `%b` was poison (with the caveat that `%b` can be generating poison itself, or it could have just passed along the poison it got from `%a`).  Now `%a` cannot be poison because we want to allow loads and stores through it as usual, which means `%b` was poison.  But that's odd -- `%b` took a non-poison value as input which it converted into poison by normal wrapping arithmetic (and there is no flag I can strip from that GEP to have it produce a safe (non-poison) value).  This means with `inrange` in the picture, non-`inbounds`, non-`inrange` GEPs are no longer "just" additions, multiplications and sign extends.
> > 
> Hmm, according to the docs:
> > Note that the inrange keyword is currently only allowed in constant getelementptr expressions.
> 
> We can punt on this I guess.
Okay.

I think the problem above still applies (i.e. right now in LLVM a non-flagged GEP can produce poison), but I do like punting.  :)


https://reviews.llvm.org/D30184