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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

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.

> 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