This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Don't create illegally-typed nodes while constant folding
ClosedPublic

Authored by frasercrmck on Mar 24 2022, 4:24 AM.

Details

Summary

This patch fixes a (seemingly very rare) crash during vector constant
folding introduced in D113300.

Normally, during legalization, if we create an illegally-typed node during
a failed attempt at constant folding it's cleaned up before being
visited, due to it having no uses.

If, however, an illegally-typed node is created during one round of
legalization and isn't cleaned up, it's possible for a second round of
legalization to create new illegally-typed nodes which add extra uses to
the old illegal nodes. This means that we can end up visiting the old
nodes before they're known to be dead, at which point we crash.

I'm not happy about this fix. Creating illegal types at all seems like a
bad idea, but we all-too-often rely on illegal constants being
successfully folded and being fixed up afterwards. However, we can't
rely on constant folding actually happening, and we don't have a
foolproof way of peering into the future.

Perhaps the correct fix is to revisit the node-iteration order during
legalization, ensuring we visit all uses of nodes before the nodes
themselves. Or alternatively we could try and clean up dead nodes
immediately after failing constant folding.

Diff Detail

Unit TestsFailed

Event Timeline

frasercrmck created this revision.Mar 24 2022, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 4:24 AM
frasercrmck requested review of this revision.Mar 24 2022, 4:24 AM
craig.topper added a comment.EditedMar 24 2022, 11:22 AM

Did this start failing because we no longer pre-check that all the build vector operands are constants like we did in llvm 13?

FoldConstantVectorArithmetic had this code

// All operands must be vector types with the same number of elements as
// the result type and must be either UNDEF or a build vector of constant
// or UNDEF scalars.
if (!llvm::all_of(Ops, IsConstantBuildVectorSplatVectorOrUndef) ||
    !llvm::all_of(Ops, IsScalarOrSameVectorSize))
  return SDValue();
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5614

legalizatoin -> legalization

craig.topper added inline comments.Mar 24 2022, 2:28 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5616

Can we use this instead of checking ISD::Constant

!isa<ConstantSDNode>(ScalarOp) && !ScalarOp.isUndef()

That allows undef(fixed the X86 test) and TargetConstant(fixes the mips changes). Does suggest that mips might be using TargetConstant in an odd way.

apply Craig's suggestion

frasercrmck marked 2 inline comments as done.Mar 25 2022, 4:35 AM

Did this start failing because we no longer pre-check that all the build vector operands are constants like we did in llvm 13?

FoldConstantVectorArithmetic had this code

// All operands must be vector types with the same number of elements as
// the result type and must be either UNDEF or a build vector of constant
// or UNDEF scalars.
if (!llvm::all_of(Ops, IsConstantBuildVectorSplatVectorOrUndef) ||
    !llvm::all_of(Ops, IsScalarOrSameVectorSize))
  return SDValue();

Yeah exactly, if I had to guess it'd be D113300 which introduced that change and was up-front about that change in behaviour. Hard to predict this kind of thing falling out of it, though.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5614

Ah, thanks!

5616

Huh, turns out we can, thanks! I regret I didn't dive deeper into those changes; I was already feeling pretty sheepish about this change. I guess we can rely on constant folding succeeding on all nodes when given either undefs and/or constants? Dunno, just feels pretty flaky to me.

frasercrmck marked 2 inline comments as done.Mar 30 2022, 2:44 AM
RKSimon accepted this revision.Mar 30 2022, 3:58 AM

LGTM - cheers

This revision is now accepted and ready to land.Mar 30 2022, 3:58 AM
frasercrmck edited the summary of this revision. (Show Details)Mar 30 2022, 5:28 AM
This revision was landed with ongoing or failed builds.Mar 30 2022, 5:29 AM
This revision was automatically updated to reflect the committed changes.
Allen added a subscriber: Allen.Mar 30 2022, 10:10 PM