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