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

[David Majnemer via Phabricator via llvm-commits]

majnemer added a comment.

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



================
Comment at: lib/IR/Instruction.cpp:126
+void Instruction::dropPoisonGeneratingFlags() {
+  switch (getOpcode()) {
+  case Instruction::Add:
----------------
sanjoy wrote:
> majnemer wrote:
> > What about the FP operations?
> None of the floating point operations are specified to generate poison.  Are you suggesting that we interpret flags like `nsz` like we interpret `nsw`?  If so, I think that's viable, but the docs need to change first.  Personally I'd say that kind of stuff is blocked on integrating the new poison semantics into LLVM.
Considering that we perform reassociation type optimizations based off those fast math flags, I think they are already defacto poison whether they want to be or not ;)

However, we shouldn't gate this change on that.


================
Comment at: lib/IR/Instruction.cpp:143
+  case Instruction::GetElementPtr:
+    cast<GetElementPtrInst>(this)->setIsInBounds(false);
+    break;
----------------
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.


https://reviews.llvm.org/D30184