[ConstantFolding] Unify handling of load from uniform value

[nikic]

Summary

There are a number of places that specially handle loads from a uniform value where all the bits are the same (zero, one, undef, poison), because we a) don't care about the load offset in that case b) it bypasses casts that might not be legal generally but do work with uniform values.

We had multiple implementations of this, with a different set of supported values each time, as well as incomplete type checks in some cases. In particular, this fixes the assertion reported in https://reviews.llvm.org/D114889#3198921, as well as a similar assertion that could be triggered via constant folding.

[ayrivera]

llvm/lib/Analysis/ConstantFolding.cpp

718

Is there a chance that Ty is not X86_MMX neither X86_AMX and C is NullValue(), but getNullValue doesn't support the type? For example, assume that Ty is a Function type, then in that case getNullValue will default into the llvm_unreachable.

722

If Ty is integer, or FP, or vector, can it be X86_AMX, X86_MMX or PtrOrPtrVector? Of not, then I think the extra check can be removed.

[nikic]

Simplify condition.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

718

I believe other problematic types cannot appear as a load or bitcast type. You can't load a function type (only a pointer to a function type).

722

Yeah, this condition was too complicated. We only need to check for int, fp or vector of them.

[ayrivera]

LGTM! Thanks for the quick fix.

llvm/lib/Analysis/ConstantFolding.cpp

718

I see, thanks for the explanation.

[paulwalker-arm]

This patch looks to be breaking the clang-aarch64-sve-vla-2stage buildbot (and probably also clang-aarch64-sve-vls-2stage). I've checked this using ./bin/clang -DNDEBUG -O3 -w -Werror=date-time -w pr19687.c && ./a.out && echo "success", which works before this patch but triggers an abort after. This happen for both AArch64 and X86.

See https://lab.llvm.org/buildbot/#/builders/198/builds/511 and https://lab.llvm.org/buildbot/#/builders/176/builds/1231.

[nikic]

@paulwalker-arm Thanks for the report, I've reverted the commit for now. I've put up D115994 to either fix the test or be told that clang generates incorrect initialization code, I'm not completely sure which it is.

[nikic]

In the meantime, I've applied https://github.com/llvm/llvm-project/commit/2926d6d335aca7e3f57ac45e6b25b1716e053fb3 as a targeted fix for the original assertion failures.

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

722

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.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

722

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.

[cameron.mcinally]

llvm/lib/Analysis/ConstantFolding.cpp

722

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.

[nikic]

llvm/lib/Analysis/ConstantFolding.cpp

722

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