Page MenuHomePhabricator

[DAGCombiner] Relax an assertion to an early return
ClosedPublic

Authored by frasercrmck on May 13 2021, 4:30 AM.

Details

Summary

The select-of-constants transform was asserting that its constant vector
inputs did not implicitly truncate their input without that as an
explicit precondition to the function. This patch relaxes that assertion
into an early return to skip the optimization.

Diff Detail

Event Timeline

frasercrmck created this revision.May 13 2021, 4:30 AM
frasercrmck requested review of this revision.May 13 2021, 4:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 4:30 AM

We got here because in foldSelectOfConstants()

auto *C1 = dyn_cast<ConstantSDNode>(N1);
auto *C2 = dyn_cast<ConstantSDNode>(N2);
if (!C1 || !C2)
  return SDValue();

didn't return.
So what are those constants?

We got here because in foldSelectOfConstants()

auto *C1 = dyn_cast<ConstantSDNode>(N1);
auto *C2 = dyn_cast<ConstantSDNode>(N2);
if (!C1 || !C2)
  return SDValue();

didn't return.
So what are those constants?

They're constants but they have implicit truncation. After legalization, RV64 -- which doesn't have legal i32 -- sees:

t9: v4i32 = BUILD_VECTOR Constant:i64<878082066>, Constant:i64<878082066>, Constant:i64<878082066>, Constant:i64<878082066>
t11: v4i32 = BUILD_VECTOR Constant:i64<1164411171>, Constant:i64<1164411171>, Constant:i64<1164411171>, Constant:i64<1164411171>

Have you investigated whether we can improve support for implicit truncation of build vector element on this path instead of just bailing out?

Have you investigated whether we can improve support for implicit truncation of build vector element on this path instead of just bailing out?

Yeah I took a look. This transform later implicitly forbids implicit truncation via its use of isAllOnesOrAllOnesSplat and isNullOrNullSplat so that would need changed. I don't see why it couldn't support implicit truncation as long as it doesn't blindly take one of the constant vector elements assuming it's the same type as the resulting vector element type. The question is also whether we can get a test case, given this combine also runs before legalization where we (RISC-V) wouldn't yet have the implicit truncation.

That said, it seems like this restriction on implicit truncation is a common idiom in the DAGCombiner -- given by the number of uses of isConstantOrConstantVector alone -- so maybe that's a larger discussion? There's so much almost-duplication in SelectionDAG for "isBuildVector", "isSplat", "isBuildOrSplat", "isConstantSplat", "isConstantSplatAllOnes", etc., that I don't think anyone wants to start untangling that web.

RKSimon accepted this revision.May 16 2021, 2:37 AM

LGTM - cheers

This revision is now accepted and ready to land.May 16 2021, 2:37 AM
This revision was automatically updated to reflect the committed changes.