[PATCH] D103458: [SLP]Improve gathering of scalar elements.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D103458#2834054 <https://reviews.llvm.org/D103458#2834054>, @rupprecht wrote:

> In D103458#2833760 <https://reviews.llvm.org/D103458#2833760>, @ABataev wrote:
>
>> In D103458#2833713 <https://reviews.llvm.org/D103458#2833713>, @rupprecht wrote:
>>
>>> We're seeing a test failure (and true miscompile AFAICT) in the CVC4 library that bisects to this patch. I don't have a nice reduction, but I can describe the issue we see.
>>>
>>> In `Constraint::externalExplain(AssertionOrder order)`, we construct a `NodeBuilder` [1]:
>>>
>>>   NodeBuilder<> nb(kind::AND);
>>>
>>> That constructor expands to this [2]:
>>>
>>>   inline NodeBuilder(Kind k) :
>>>     d_nv(&d_inlineNv),
>>>     d_nm(NodeManager::currentNM()),
>>>     d_nvMaxChildren(nchild_thresh) {
>>>     Assert(k != kind::NULL_EXPR && k != kind::UNDEFINED_KIND)
>>>         << "illegal Node-building kind";
>>>   
>>>     d_inlineNv.d_id = 1; // have a kind already
>>>     d_inlineNv.d_rc = 0;
>>>     d_inlineNv.d_kind = expr::NodeValue::kindToDKind(k);
>>>     d_inlineNv.d_nchildren = 0;
>>>   }
>>>
>>> `d_inlineNv` is a local class data member, and `d_nv` by default is just a pointer to that data member (but can be reassigned to point to something heap allocated)
>>>
>>> The fields of `d_inlineNv` should be zero except for `d_id` which is 1, and `d_kind` which is 21 (corresponding to `kind::AND`). However after this commit, the struct is initialized with poison. The IR diff we see is this:
>>>
>>>   define dso_local void @_ZNK4CVC46theory5arith10Constraint15externalExplainEm(%"class.CVC4::NodeTemplate"* noalias sret(%"class.CVC4::NodeTemplate") align 8 %agg.result, %"class.CVC4::theory::arith::Constraint"* nonnull align 8 dereferenceable(144) %this, i64 %order) local_unnamed_addr #15 align 32 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
>>>   ...
>>>   -  %40 = phi <2 x i64> [ %phi.bo220, %37 ], [ <i64 1, i64 21>, %if.else69 ]
>>>   +  %40 = phi <2 x i64> [ %phi.bo220, %37 ], [ poison, %if.else69 ]
>>>   ...
>>>     %42 = bitcast %"class.CVC4::NodeBuilder.623"* %nb to <2 x i64>*
>>>     store <2 x i64> %40, <2 x i64>* %42, align 16
>>>
>>> (`%phi.bo220` is a path never taken AFAICT)
>>>
>>> [1] https://github.com/CVC4/CVC4-archived/blob/40eac7f0529176bcc8464d6c4c8804fbde628c2b/src/theory/arith/constraint.cpp#L1563
>>> [2] https://github.com/CVC4/CVC4-archived/blob/40eac7f0529176bcc8464d6c4c8804fbde628c2b/src/expr/node_builder.h#L377
>>
>> Hi, really need a reproducer and a command line.
>
> Understood -- this is just an initial report with some early analysis; I've occasionally gotten lucky in reporting bugs.
>
>> Otherwise, it is hard to understand why there is a miscompilation.
>
> I fully agree. And the IR code around the change doesn't have any of the usual shufflevector/insertelement/extractelement instructions, so it doesn't look like SLP is really involved much here. But to clarify, I really do mean that it bisects to this patch -- if I build clang at 0120e6c295e42d3b9ed2cd125b1c9056a59fbcf6 <https://reviews.llvm.org/rG0120e6c295e42d3b9ed2cd125b1c9056a59fbcf6> (one commit prior to this), the generated IR looks correct and the test passes. My hunch is that SLP just makes some valid optimization that exposes a miscompilation elsewhere.

Yes, this is possible but we have to be absolutely sure. I would really appreciate it if you could provide a reproducer.

>> This is rather strange that this patch could cause such miscompilation, I would suppose that a https://github.com/llvm/llvm-project/commit/f126e8ec2873ceedde53d2ccee1a66a83620e9a6#diff-e3b933e8c46156c0eaf7cbb67866b712f69a8484bc941d10acec9d4d17b9f061 could cause this.
>
> Reverting that patch locally does not help, but thanks for the suggestion.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103458