Page MenuHomePhabricator

[DAGCombiner] Don't create sexts of deleted xors when they were in-visit replaced
ClosedPublic

Authored by laytonio on Dec 14 2020, 9:44 PM.

Details

Summary

Fixes a bug introduced by D91589.

When folding (sext (not i1 x)) -> (add (zext i1 x), -1), we try to replace the not first when possible. If we replace the not in-visit, then the now invalidated node will be returned, and subsequently we will return an invalid sext. In cases where the not is replaced in-visit we can simply return SDValue, as the not in the current sext should have already been replaced.

Thanks @jgorbe, for finding the below reproducer.

The following reduced test case crashes clang when built with clang -O1 -frounding-math:

template <class> class a {
  int b() { return c == 0.0 ? 0 : -1; }
  int c;
};
template class a<long>;

A debug build of clang produces this "assertion failed" error:

clang: /home/jgorbe/code/llvm/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:264: void {anonymous}::DAGCombiner::AddToWorklist(llvm::
SDNode*): Assertion `N->getOpcode() != ISD::DELETED_NODE && "Deleted Node added to Worklist"' failed.

Diff Detail

Event Timeline

laytonio created this revision.Dec 14 2020, 9:44 PM
laytonio requested review of this revision.Dec 14 2020, 9:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 9:44 PM
spatel added a comment.EditedDec 15 2020, 5:15 AM

We need a codegen test to verify that the crash is fixed. I used -emit-llvm on the included example to get this (please reduce further if possible):

target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.15.0"

%class.a = type { i32 }

define i32 @_ZN1aIlE1bEv(%class.a* nonnull dereferenceable(4) %this) {
entry:
  %c = getelementptr inbounds %class.a, %class.a* %this, i64 0, i32 0
  %0 = load i32, i32* %c, align 4, !tbaa !3
  %conv = tail call double @llvm.experimental.constrained.sitofp.f64.i32(i32 %0, metadata !"round.dynamic", metadata !"fpexcept.ignore") #2
  %cmp = tail call i1 @llvm.experimental.constrained.fcmp.f64(double %conv, double 0.000000e+00, metadata !"oeq", metadata !"fpexcept.ignore") #2
  %not.cmp = xor i1 %cmp, true
  %cond = sext i1 %not.cmp to i32
  ret i32 %cond
}
RKSimon added inline comments.Dec 15 2020, 8:28 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10724–10735

Instead of visitXOR() - do we need a visitXORLike op instead here? Or maybe this should be handled by SimplifyDemandedBits?

laytonio updated this revision to Diff 312018.Dec 15 2020, 2:01 PM

Add test case.

laytonio added inline comments.Dec 15 2020, 2:05 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10724–10735

I was planning on trying to extend extractBooleanFlip to include more of the boolean fold and use that here as a follow up.

spatel added inline comments.Dec 21 2020, 8:51 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10724–10735

@RKSimon - are you ok with this extension to solve the crash, or should we change course as suggested (and revert D91589 to solve the crash)?

RKSimon added inline comments.Dec 21 2020, 9:48 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10724–10735

As a first step I'm OK with this going in, but I think creating a visitXORLike helper would be better.

spatel accepted this revision.Dec 22 2020, 9:14 AM

LGTM - there's also a code pattern like this in DAGCombiner::rebuildSetCC(), so it's hopefully safe. But as noted, we can probably do better by changing the structure a bit.

This revision is now accepted and ready to land.Dec 22 2020, 9:14 AM

Thanks for the review! Could you please commit this for me? Layton Kifer <laytonkifer@gmail.com>

Thanks for the review! Could you please commit this for me? Layton Kifer <laytonkifer@gmail.com>

Done, and confirmed that the original (unreduced) test case is now building successfully. Thanks!