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

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:722
+      (Ty->isIntOrIntVectorTy() || Ty->isFPOrFPVectorTy()))
+    return Constant::getAllOnesValue(Ty);
+  return nullptr;
----------------
cameron.mcinally wrote:
> 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.
I've landed a partial fix in https://github.com/llvm/llvm-project/commit/930a68765dff96927d706d258ef0c2ad9c7ec2ab, because this was checking the wrong type sizes.

I plan to also add handling for this in the constant folding code though, to also fix this variant of the problem: https://github.com/llvm/llvm-project/commit/659871cede9e3475c5de986ba4cace58e70f4801#diff-cc91356612b63dff2481358f87d5da7e98d7bbf8fc65c80e55d55c20b1dba462


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115924