Page MenuHomePhabricator

[DAG] make binops with undef operands consistent with IR
ClosedPublic

Authored by spatel on Feb 9 2018, 1:40 PM.

Details

Summary

This started by noticing that scalar and vector types were producing different results with div ops in PR36305:
https://bugs.llvm.org/show_bug.cgi?id=36305

...but the problem is bigger. I couldn't keep it straight without a table, so I'm attaching that as a PDF to this review. The x86 tests in undef-ops.ll correspond to that table.

Green means that instsimplify and the DAG agree on the result for all types.
Red means the DAG was returning undef when IR was not.
Yellow means the DAG was returning a non-undef result when IR returned undef.

This patch assumes that we're doing the right thing in IR, but now's a good time to confirm that. :)

Note: I couldn't find any problems with creating vector constants as the code comments were warning, but those comments were written long ago in rL36413 .

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Feb 9 2018, 1:40 PM

Some of the test changes are a little iffy... we might want to adjust the to stop using undef rather than just change the expected result, to try to preserve some of the original intent.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
4713 ↗(On Diff #133673)

"For vectors, we can't easily build an all zero vector" doesn't mean that it's hard to construct a BUILD_VECTOR; the problem is ensuring we lower it before we run selection. But it might not be a problem anymore because DAGCombine calls the legalizer?

spatel updated this revision to Diff 133750.Feb 10 2018, 7:53 AM

Patch updated:
Edited tests so they are not affected by this patch:
rL324814
rL324816
rL324817

I think the remaining x86 tests show the undef functional change from this patch as intended.
I'm not sure what to do with the Hexagon test - changing some constants as shown didn't restore the current CHECK. @kparzysz - suggestions?

oh, wow, so many bugs in DAG combiner handling undef.. Nice table btw!

I wonder if the handling of nsw/nuw in SelectionDAG isn't buggy as well, but that's another question..

regehr added a subscriber: regehr.Feb 11 2018, 7:29 AM

The change in the Hexagon test is fine. The original testcase crashed before the fix, so as long as it compiles successfully, any CHECK will work.

This revision is now accepted and ready to land.Feb 12 2018, 11:41 AM
This revision was automatically updated to reflect the committed changes.