This is an archive of the discontinued LLVM Phabricator instance.

[DAG] vector div/rem with any zero element in divisor is undef
ClosedPublic

Authored by spatel on Mar 10 2017, 6:53 AM.

Details

Summary

This is the backend counterpart to:
https://reviews.llvm.org/rL297390
https://reviews.llvm.org/rL297409
and follow-up to:
https://reviews.llvm.org/rL297384

It surprised me that we need to duplicate the check in FoldConstantArithmetic and FoldConstantVectorArithmetic, but one or the other doesn't catch all of the test cases. There is an existing code comment about merging those someday. If we fix more cases like this to properly recognize undef (signed overflow in sdiv, oversized shifts, etc), we should fix the code structure or at least make a helper function to avoid repeating code.

Diff Detail

Event Timeline

spatel created this revision.Mar 10 2017, 6:53 AM
efriedma edited edge metadata.Mar 10 2017, 11:26 AM

I agree it's weird that there are two functions for constant folding... but you can leave it for now, I guess.

Maybe check for null or undef, rather than just null?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
3858

I think Ops[1] must be a BUILD_VECTOR here, but it isn't obvious from the structure of the code. Maybe explicitly cast<> it to BuildVectorSDNode?

spatel updated this revision to Diff 91560.Mar 13 2017, 7:46 AM
spatel marked an inline comment as done.

Patch updated:

  1. The code duplication was already bugging me, and we know it's only going to grow over time, so I'm proposing a single place to house the undef checking. Fixing the duplication between FoldConstantArithmetic and FoldConstantVectorArithmetic is another step.
  2. Fixed to check for zero or undef elements in the build vector.
efriedma accepted this revision.Mar 13 2017, 3:56 PM

LGTM with a minor tweak.

include/llvm/CodeGen/SelectionDAG.h
744

This is a little unclear... maybe "return true if the result of this operation is always undefined"?

This revision is now accepted and ready to land.Mar 13 2017, 3:56 PM

I agree it's weird that there are two functions for constant folding... but you can leave it for now, I guess.

Maybe check for null or undef, rather than just null?

include/llvm/CodeGen/SelectionDAG.h
744

Yep - that's better. Thanks!

This revision was automatically updated to reflect the committed changes.