Page MenuHomePhabricator

[IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections.
ClosedPublic

Authored by AndrewLitteken on Jun 11 2021, 12:05 PM.

Details

Summary

When the initial relationship between two pairs of values between similar sections is ambiguous to commutativity, arguments to the outlined functions can be passed in such that the order is incorrect, causing miscompilations. This adds a canonical mapping to each similarity section, so that we can maintain the relationship of global value numbering from one section to another.

Added Tests:
Transforms/IROutliner/outlining-commutative-operands-opposite-order.ll
unittests/Analysis/IRSimilarityIdentifierTest.cpp - IRSimilarityCandidate:CanonicalNumbering

Diff Detail

Unit TestsFailed

TimeTest
164,890 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases/Linux::uar_signals.cpp
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -std=c++11 -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/Linux/uar_signals.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Linux/Output/uar_signals.cpp.tmp -pthread && /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Linux/Output/uar_signals.cpp.tmp

Event Timeline

AndrewLitteken requested review of this revision.Jun 11 2021, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 12:05 PM
ormris removed a subscriber: ormris.Jun 11 2021, 2:49 PM

Updating the diff for clang-format errors.

jroelofs added inline comments.Aug 20 2021, 1:55 PM
llvm/lib/Analysis/IRSimilarityIdentifier.cpp
586–591

I'd sink this closer to where it's used so there's less live range to think about.

llvm/lib/Transforms/IPO/IROutliner.cpp
855–858
llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
2015

alternatively

2017

ASSERT_EQ(x, y); will give better diagnostics when it fails than ASSERT_TRUE(x == y); does.

2049

ASSERT_NE. Also, should it check what's in the set that was found?

AndrewLitteken added inline comments.Aug 20 2021, 2:11 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
855–858
paquette added inline comments.Tue, Aug 24, 1:06 PM
llvm/include/llvm/Analysis/IRSimilarityIdentifier.h
573

break up run-on?

580
  • Use a more descriptive name than A?
  • Maybe createCanonicalMappingFor would be a better name?
582

sentence is kind of long, break it up?

595

more descriptive name than A?

661

doxygen comment?

668

doxygen comment?

llvm/lib/Analysis/IRSimilarityIdentifier.cpp
759

do you need this here?

802

do we need this here?

yroux added a subscriber: yroux.Wed, Aug 25, 2:03 AM

Apart from the previous comments, this patch LGTM and fixes a correctness issue observed on Spec, Thanks for that

Updating for comments

paquette accepted this revision.Fri, Aug 27, 1:15 PM

LGTM after fixing nits on comments.

llvm/lib/Analysis/IRSimilarityIdentifier.cpp
765

Can you change this comment to explain why you're iterating over the mappings?

772

Can you explain why?

This revision is now accepted and ready to land.Fri, Aug 27, 1:15 PM
This revision was landed with ongoing or failed builds.Fri, Aug 27, 3:04 PM
This revision was automatically updated to reflect the committed changes.