Page MenuHomePhabricator

[DAG][PowerPC] Fix dropped `nsw` flag in `SimplifySetCC` by adding `doesNodeExist` helper
ClosedPublic

Authored by lkail on Oct 22 2020, 1:42 AM.

Details

Summary

SimplifySetCC invokes getNodeIfExists without passing Flags argument and getNodeIfExists uses a default SDNodeFlags to intersect the original flags, as a consequence, flags like nsw is dropped. Added a new helper function doesNodeExist to check if a node exists without modifying its flags.

Diff Detail

Unit TestsFailed

TimeTest
360 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

lkail created this revision.Oct 22 2020, 1:42 AM
lkail requested review of this revision.Oct 22 2020, 1:42 AM
qiucf added a comment.Oct 22 2020, 3:17 AM

This patch can fix the issue reflected by your case. However, if you call getNodeIfExists somewhere else with argument Flags, I guess you will still meet the problem that flags of the unrelated existing node are modified.

The optional argument Flags looks confusing sometimes: does it mean (1) 'I want the node with the same opcodes, operands, and flags' or (2) 'I want the node with the same opcodes and operands, apply flags to it if found'?

If (1), the node id doesn't put flags into consideration (also to getNode). If (2), why use intersectFlags and modifying the original node still looks not reasonable.

I think getNodeIfExists() should have the same semantics with respect to flags as getNode().

Maybe we can add a new helper "doesNodeExist()" which just returns a boolean, and doesn't mess with the flags?

lkail updated this revision to Diff 300568.Oct 25 2020, 7:56 PM
lkail retitled this revision from [DAG][PowerPC] Do not overwrite flags in `getNodeIfExists` without passing `Flags` argument to [DAG][PowerPC] Fix dropped `nsw` flag in `SimplifySetCC` by adding `doesNodeExist` helper.
lkail edited the summary of this revision. (Show Details)

I think what @efriedma suggests is viable and eases the confusion mentioned by @qiucf .

lkail updated this revision to Diff 300574.Oct 25 2020, 9:10 PM
lkail added a comment.Nov 24 2020, 1:52 AM

Gentle ping.

nemanjai accepted this revision.Nov 24 2020, 3:24 AM

LGTM. This seems like a reasonable solution.

This revision is now accepted and ready to land.Nov 24 2020, 3:24 AM
lkail updated this revision to Diff 307501.Nov 24 2020, 7:33 PM

Reformat code.