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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
