Keep the shadow of a constant a constant in invertPointerM - #3137
Conversation
wsmoses
left a comment
There was a problem hiding this comment.
this doesn't feel like the right fix, why is it not a constant in some cases?
|
@wsmoses pushed an update. I'd guess the first commit should be dropped, just left it while you're reviewing. But also feel free to close here and fix it yourself. Shortened AI answer: The value is fine — the TypeTree is empty. invertPointerM's GlobalVariable case inverts an initializer using the tree it holds for the pointer, which says nothing about the An all-Unknown tree is the one input the predicates disagree on — anyFloat() says "may be float", allFloat() says "not all float" — which is exactly the guard on the Type analysis already knows: TR.query() returns {[-1]:Anything}, which slices to Anything per element, and the existing isa && allFloat(anythingIsFloat) case if (!TT.isKnown() && isa(oval)) This alone fixes #3128 — with commit 1's fold disabled both reproducers pass and @tab_shadow is zeroinitializer either way. I kept the fold as a backstop for constants that Across the 1205 lit tests the new branch is reached by 1 file (the new test), and nothing else changes. |
The ConstantDataArray / ConstantArray / ConstantStruct / ConstantVector cases of invertPointerM invert each element and feed the results straight into ConstantArray::get and friends, so they do cast<Constant>() on whatever the recursive call returned. That call is not guaranteed to hand back a constant: when the element's TypeTree says it overlaps a float but does not cover all of it, invertPointerM falls through to the generic partially-float path, which emits an alloca/store/store-zero/load sequence and returns the LoadInst. The cast then trips Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' in llvm::cast<llvm::Constant>. A partial float window is exactly what the individual i8s of a [N x i8] blob holding packed doubles produce. Differentiating librint, a Rust `static` of type [16 x i8] holding f64 tables reaches invertPointerM as the initializer of a global, recurses into the ConstantDataArray case, and inverting the element `i8 85` (byte 0 of 0x3FD5555555555555) returns a LoadInst. Fix it at the source rather than at the four call sites: when the value being inverted is a scalar constant, compute the same byte-wise nulling by folding instead of by emitting instructions. nullFloatBytesOfConstant() zeroes every byte the TypeTree types as a float (or as anything other than an integer or a pointer) and keeps the rest, which is what the alloca sequence does, and returns null for anything it cannot reinterpret bitwise so those inputs still take the old path. None of the 1204 existing lit tests reach the new branch, so this only affects inputs that used to assert. Closes EnzymeAD#3128
The previous commit stopped invertPointerM handing a LoadInst back to its own cast<Constant> callers, but it left the reason a constant reached the runtime path in the first place. This addresses that. invertPointerM's GlobalVariable case inverts a global's initializer with the TypeTree it holds for the *pointer*, which says nothing about the pointee's contents, and the ConstantDataArray / ConstantArray / ConstantStruct / ConstantVector cases then slice that empty tree per element. Instrumenting the EnzymeAD#3128 reproducer shows the aggregate arriving with TypeTree {} and every element sliced to {} as well, so nothing about them can be classified. An entirely Unknown tree is the one input the two predicates disagree on: anyFloat() reads Unknown as "may be a float" and returns true, allFloat() reads it as "not all float" and returns false. That combination is exactly the guard on the generic partially-float path, so a compile-time constant with no type information is routed into emitting an alloca/store/store-zero/load. Type analysis already has the answer. On the same reproducer TR.query() gives {[-1]:Anything} for the aggregate, which slices to Anything per element, and the existing `isa<ConstantData> && allFloat(anythingIsFloat)` case then folds each element to a null constant. So query it whenever the incoming tree carries no information and the value is a constant. This alone fixes EnzymeAD#3128: with the previous commit's fold disabled, both the reduced and the unreduced reproducer go from the assertion to exit 0, and @tab_shadow comes out `zeroinitializer` either way. The fold from the previous commit is kept as a backstop for constants that type analysis genuinely types as partially float, where the byte-wise answer is not simply zero.
0b9d2fd to
d8b51ef
Compare
Rust always emits arrays as
[N x i8], which might be why it showed up repeatedly.The ConstantDataArray / ConstantArray / ConstantStruct / ConstantVector cases of invertPointerM invert each element and feed the results straight into ConstantArray::get and friends, so they do cast() on whatever the recursive call returned. That call is not guaranteed to hand back a constant: when the element's TypeTree says it overlaps a float but does not cover all of it, invertPointerM falls through to the generic partially-float path, which emits an alloca/store/store-zero/load sequence and returns the LoadInst. The cast then trips
Assertion `isa(Val) && "cast() argument of incompatible type!"'
in llvm::castllvm::Constant.
A partial float window is exactly what the individual i8s of a [N x i8] blob holding packed doubles produce. Differentiating a Rust
staticof type [16 x i8] holding f64 tables reaches invertPointerM as the initializer of a global, recurses into the ConstantDataArray case, and inverting the elementi8 85(byte 0 of 0x3FD5555555555555) returns a LoadInst.Fix it at the source rather than at the four call sites: when the value being inverted is a scalar constant, compute the same byte-wise nulling by folding instead of by emitting instructions. nullFloatBytesOfConstant() zeroes every byte the TypeTree types as a float (or as anything other than an integer or a pointer) and keeps the rest, which is what the alloca sequence does, and returns null for anything it cannot reinterpret bitwise so those inputs still take the old path.
None of the 1204 existing lit tests reach the new branch, so this only affects inputs that used to assert.
@wsmoses
Closes #3128