Page MenuHomePhabricator

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

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

Details

Reviewers
jonpa
spatel
qiucf
efriedma
uweigand
nemanjai
Group Reviewers
Restricted Project
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.Sun, Oct 25, 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.Sun, Oct 25, 9:10 PM
lkail added a comment.Tue, Nov 24, 1:52 AM

Gentle ping.

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

LGTM. This seems like a reasonable solution.

This revision is now accepted and ready to land.Tue, Nov 24, 3:24 AM