This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Forbid undef folds in expandBinOp
ClosedPublic

Authored by nikic on Aug 10 2020, 1:23 PM.

Details

Summary

This is the replacement for D84250 based on D84792. As we recursively fold with the same value twice, we need to disable undef folds, to prevent an undef from being folded to two different values.

Reverting the revert rG00f3579aea6e3d4a4b7464c3db47294f71cef9e4 and using the test case from https://reviews.llvm.org/D83360#2145793, it no longer performs the incorrect fold.

Diff Detail

Event Timeline

nikic created this revision.Aug 10 2020, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2020, 1:23 PM
nikic requested review of this revision.Aug 10 2020, 1:23 PM
aqjune added a comment.EditedAug 10 2020, 6:22 PM

Does this patch need an update at llvm/test/Transforms/InstSimplify/select.ll as well? (relevant commit: https://github.com/llvm/llvm-project/commit/566a66703f020996d07191323ae8ad6f7ad827b3 )


Nevermind, it should be updated after the select undef fold is disabled.

aqjune accepted this revision.Aug 10 2020, 6:26 PM

I think this patch is straightforward. I'll accept this; could anyone have a look as well?

This revision is now accepted and ready to land.Aug 10 2020, 6:26 PM

Why is the test not part of this?

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

That's right. I only added the test to make sure we don't miscompile it once the select fold is disabled. I don't know of a way to directly test this change right now.

jdoerfert accepted this revision.Aug 11 2020, 6:32 AM

Why is the test not part of this?

Maybe it is because an operation on undef is already folded before expandBinOp is reached. Is it right @nikic? I think having a working test here likely means we found another miscompilation regarding undef and distributivity law.
If select c, undef, x --> x is disabled later, it will leave the undef constant, making the test at select.ll valid.

That's right. I only added the test to make sure we don't miscompile it once the select fold is disabled. I don't know of a way to directly test this change right now.

That's unfortunate but the change looks harmless enough I guess.

This revision was landed with ongoing or failed builds.Aug 11 2020, 9:39 AM
This revision was automatically updated to reflect the committed changes.