[PATCH] D122382: [SelectionDAG] Don't create illegally-typed nodes while constant folding
Fraser Cormack via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 04:35:21 PDT 2022
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
More information about the llvm-commits
mailing list