Page MenuHomePhabricator
Differential D145472

[LV][AArch64] Resolve test failure due use of unordered container
AcceptedPublic
Actions

Authored by sushgokh on Mon, Mar 6, 10:30 PM.

Details

Reviewers
fhahn
SjoerdMeijer
madhur13490
sdesmalen
Summary

D143422 test AArch64/reg-usage.ll has an issue with the output ordering due to use of unordered container. This was discovered by
-DLLVM_REVERSE_ITERATION:BOOL=ON cmake option.

This patch tries to address it by making use of ordered container.

Diff Detail

Unit TestsFailed
View All

TimeTest
60,020 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest

Event Timeline

sushgokh created this revision.Mon, Mar 6, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 6, 10:30 PM
Herald added subscribers: StephenFan, hiraditya, kristof.beyls. · View Herald Transcript
sushgokh requested review of this revision.Mon, Mar 6, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 6, 10:30 PM
Herald added subscribers: llvm-commits, pcwang-thead. · View Herald Transcript
sushgokh retitled this revision from [LV] Resolve test failure due use of unordered container to [LV][AArch64] Resolve test failure due use of unordered container.Mon, Mar 6, 10:30 PM
sushgokh mentioned this in D143422: [LV] Update logic for calculating register usage due to invariants.Mon, Mar 6, 10:35 PM
Harbormaster completed remote builds in B217801: Diff 502928.Tue, Mar 7, 12:20 AM
sdesmalen accepted this revision.Tue, Mar 7, 1:20 AM
This revision is now accepted and ready to land.Tue, Mar 7, 1:20 AM
fhahn accepted this revision.Tue, Mar 7, 4:38 AM

LGTM, thanks! The commit title could be a bit more descriptive by explicitly mentioning what the code change is, e.g. something like: [LV] Use SetVector to collect invariants in calculateRegisterUsage.

Also, this is not AArch64 specific, so it would be good to drop the [AArch64] part.

sushgokh added a comment.Wed, Mar 8, 1:51 AM
In D145472#4174810, @fhahn wrote:

LGTM, thanks! The commit title could be a bit more descriptive by explicitly mentioning what the code change is, e.g. something like: [LV] Use SetVector to collect invariants in calculateRegisterUsage.

Also, this is not AArch64 specific, so it would be good to drop the [AArch64] part.

Thanks @fhahn for the suggestion.

Regarding inclusion of AArch64 in title, I thought this was more about AArch64 test failure. But I was incorrect. Will keep this in mind that its more about the change

Revision Contents
Changeset List

PathSize
llvm/
lib/
Transforms/
Vectorize/
2 lines

Diff 502928

View Options

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

Loading...