[PATCH] D115924: [ConstantFolding] Unify handling of load from uniform value

[Cameron McInally via Phabricator via cfe-commits]

cameron.mcinally added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:722
+      (Ty->isIntOrIntVectorTy() || Ty->isFPOrFPVectorTy()))
+    return Constant::getAllOnesValue(Ty);
+  return nullptr;
----------------
nikic wrote:
> cameron.mcinally wrote:
> > Sorry for the late comment, but is this legal to do if the src and dest types are different sizes? E.g.:
> > 
> > ```
> > %xxx_cast = bitcast i8* %xxx to i1*
> > store i1 true, i1* %xxx_cast
> > %yyy = load i8, i8* %xxx
> > ```
> > 
> > In this case, we'll be turning an i1 -1 into an i8 -1, which changes bits.
> This code assumes that the loaded type is either smaller or that loading a larger type fills in the remaining bits with poison, so we can use any value for them. The caller is responsible for doing a type size check if necessary.
> 
> However, I don't believe that non-byte-sized types were really considered either here or in other parts of the constant load folding code. In that case the type store sizes are the same, but the type sizes differ.
> 
> Now, as to whether this behavior is actually incorrect, LangRef has the following to say on non-byte-sized memory accesses:
> 
> > When writing a value of a type like i20 with a size that is not an integral number of bytes, it is unspecified what happens to the extra bits that do not belong to the type, but they will typically be overwritten.
> 
> > When loading a value of a type like i20 with a size that is not an integral number of bytes, the result is undefined if the value was not originally written using a store of the same type.
> 
> Based on a strict reading, I believe the store of i1 and load of i8 would result in the remaining bits having an unspecified, but generally non-poison value. The reverse would be UB (which really doesn't make sense to me -- it would be great if we could rework this to be more well-defined.)
> 
> So, yeah, I'd say this is a bug. I'll take a look.
Thanks, Nikita.

Looking at the LangRef language, I suspect that you're correct here:

```
Based on a strict reading, I believe the store of i1 and load of i8 would result in the remaining bits having an unspecified, but generally non-poison value. 
```

Requiring the IR producer to maintain those unspecified bits is an acceptable answer. ;)

I wish LangRef took the responsibility of maintaining the unspecified i1/i4 bits off of the IR producer, since they're so common in predication, but I also understand the access instruction limitations as well. It's an unfortunate situation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115924