[PATCH] D122382: [SelectionDAG] Don't create illegally-typed nodes while constant folding

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 2 inline comments as done.
frasercrmck added a comment.

In D122382#3406064 <https://reviews.llvm.org/D122382#3406064>, @craig.topper wrote:

> 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 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5614
+        // fail to constant fold we can't guarantee the (dead) nodes we're
+        // creating will be cleaned up before being visited for legalizatoin.
+        if (NewNodesMustHaveLegalTypes &&
----------------
craig.topper wrote:
> legalizatoin -> legalization
Ah, thanks!


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5616
+        if (NewNodesMustHaveLegalTypes &&
+            ScalarOp.getOpcode() != ISD::Constant &&
+            TLI->getTypeAction(*getContext(), InSVT) !=
----------------
craig.topper wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122382