It is proper to relax non-negative limitation of step_vector. Also this patch adds more combines for step_vector:
(sub X, step_vector(C)) -> (add X, step_vector(-C))
TestPlan: check-llvm
Differential D100812
[DAGCombiner] Allow operand of step_vector to be negative. junparser on Apr 19 2021, 9:05 PM. Authored by
Details It is proper to relax non-negative limitation of step_vector. Also this patch adds more combines for step_vector: TestPlan: check-llvm
Diff Detail
Event TimelineComment Actions I've nothing against the change but it is more involved than updating the comment and assert. There are places where STEP_VECTOR's operand is treated as unsigned, based on the original requirement, that will need to be updated. For example DAGTypeLegalizer::PromoteIntRes_STEP_VECTOR. Ideally we'd want to add/update tests to show the signedness is properly protected during type legalisation. Comment Actions I also have nothing against the change in principle, but in addition to @paulwalker-arm's comments, RISC-V won't support this: it expects IMM to be 1, as it always was before this. We shouldn't introduce something that regresses this target, so the lowering of STEP_VECTOR will need to be extended to legalize/lower the operation. Comment Actions Hmm I just saw that D100088 went in without anyone involved with RISC-V being on the reviewer list or notified. I think that's technically a regression since the RISC-V backend crashes on all those test cases that were added.
Comment Actions A couple of minor requests plus it's worth adding a "step-vector has more than one use" test but otherwise looks good to me. Thanks for your efforts @junparser, also thanks @frasercrmck and @craig.topper for the assists.
|
nit: double space.