This is an archive of the discontinued LLVM Phabricator instance.

[DAG] recognize div/rem by 0 as undef before trying constant folding
ClosedPublic

Authored by spatel on Mar 8 2017, 8:36 AM.

Details

Summary

As discussed in the review thread for rL297026, this is actually 2 changes that would independently fix all of the test cases in the patch:

  1. Return undef in FoldConstantArithmetic for div/rem by 0.
  2. Move basic undef simplifications for div/rem (simplifyDivRem()) before foldBinopIntoSelect() as a matter of efficiency.

I can split it up, but I'm not sure yet how to expose the failures once either one of the fixes goes in.

I will handle the case of vectors with any zero element as a follow-up unless we want to do it all in one step. That change is the DAG sibling for D30665 + adding a check of vector elements to FoldConstantVectorArithmetic().

There are likely more improvements for undef handling needed for functions like isConstantSplatVector(), so I can also try to fix those too.

Diff Detail

Event Timeline

spatel created this revision.Mar 8 2017, 8:36 AM
spatel updated this revision to Diff 91029.Mar 8 2017, 9:12 AM
spatel edited the summary of this revision. (Show Details)

Patch updated:
Changing the definition of isConstantOrConstantVector() to include undef may have unintended consequences (there's a test in test/CodeGen/ARM/select.ll that will trigger the assert in foldBinOpIntoSelect()), so just assert that an undef result from DAG.getNode() is ok for now.

spatel updated this revision to Diff 91038.Mar 8 2017, 9:44 AM
spatel added a subscriber: zvi.

Patch updated:
I didn't notice that an extra x86 test (for https://bugs.llvm.org/show_bug.cgi?id=30693) was failing with this change because it has div-by-0.
I can't tell if that test is useful or not any more ( cc'ing @zvi ).

efriedma edited edge metadata.Mar 8 2017, 11:01 AM

I doubt pr30693.ll is still usefully testing anything with this patch... this is the problem with bugpoint-reduced tests. Needs to either be fixed or deleted.

Looks good otherwise.

zvi added a comment.Mar 8 2017, 11:52 AM

I agree this test is no longer valuable. I'm fine with removing it.

This revision was automatically updated to reflect the committed changes.