Page MenuHomePhabricator

[clang] Add mustprogress and llvm.loop.mustprogress attribute deduction
Needs ReviewPublic

Authored by atmnpatel on Aug 29 2020, 1:38 PM.

Details

Summary

Since C++11, the C++ standard has a forward progress guarantee
[intro.progress], so all such functions must have the mustprogress
requirement. In addition, from C11 and onwards, loops without a non-zero
constant conditional or no conditional are also required to make
progress (C11 6.8.5p6). This patch implements these attribute deductions
so they can be used by the optimization passes.

Diff Detail

Unit TestsFailed

TimeTest
400 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
20 mswindows > Clang.CodeGen::attr-mustprogress-0.cpp
Script: -- : 'RUN: at line 2'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\clang.exe++ -std=c++98 -S -emit-llvm C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGen\attr-mustprogress-0.cpp -o - | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGen\attr-mustprogress-0.cpp
90 mswindows > Clang.CodeGen::attr-mustprogress-1.cpp
Script: -- : 'RUN: at line 2'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\clang.exe --driver-mode=g++ -std=c++11 -S -emit-llvm C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGen\attr-mustprogress-1.cpp -o - | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGen\attr-mustprogress-1.cpp
110 mswindows > LLVM.tools/llvm-symbolizer/pdb::missing_pdb.test
Script: -- : 'RUN: at line 1'; not c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llvm-symbolizer.exe 0x401000 0x401001 -obj="C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer\pdb/Inputs/missing_pdb.exe" 2>C:\ws\w16n2-1\llvm-project\premerge-checks\build\test\tools\llvm-symbolizer\pdb\Output\missing_pdb.test.tmp.err | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer\pdb\missing_pdb.test

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
atmnpatel retitled this revision from [clang] Adds noprogress attribute deduction for infinite loops to [clang] Add noprogress attribute deduction for infinite loops.Aug 29 2020, 5:21 PM
aaron.ballman added inline comments.Aug 31 2020, 6:24 AM
clang/lib/CodeGen/CGLoopInfo.h
211

I'd drop the top-level const on the declaration of NoProgress (that's not a style we typically use in the project).

atmnpatel updated this revision to Diff 289821.EditedSep 3 2020, 3:26 PM

Renamed llvm loop metadata, changed the way it is deduced. Fixed broken test.

atmnpatel marked an inline comment as done.Sep 3 2020, 3:27 PM
atmnpatel added inline comments.
clang/lib/CodeGen/CGLoopInfo.h
211

My bad, thanks!

atmnpatel marked 2 inline comments as done.Sep 3 2020, 3:28 PM
atmnpatel updated this revision to Diff 289849.Sep 3 2020, 8:52 PM

In Summary:

  • I changed the llvm loop metadata name to mustprogress, to indicate that loops with this attribute are required to have side-effects.
  • The mayprogress function attribute is applied to functions where an infinite loop is found with a constant conditional.
  • The mustprogress attribute is applied to loops within a mayprogress function that do not have a constant conditional.
atmnpatel retitled this revision from [clang] Add noprogress attribute deduction for infinite loops to [clang] Add mayprogress and llvm.loop.mustprogress attribute deduction.Sep 3 2020, 10:00 PM
atmnpatel edited the summary of this revision. (Show Details)
atmnpatel updated this revision to Diff 289997.Sep 4 2020, 11:03 AM

Renamed mayprogress to maynotprogress.

atmnpatel updated this revision to Diff 291249.Sep 11 2020, 9:29 AM

More tightly follows both the C and C++ standards. maynotprogress is only added to C functions when compiled with C11 or later.

uenoku added a subscriber: uenoku.Sep 11 2020, 9:46 AM
atmnpatel updated this revision to Diff 291471.Sep 13 2020, 1:00 PM

Flipped direction.

atmnpatel retitled this revision from [clang] Add mayprogress and llvm.loop.mustprogress attribute deduction to [clang] Add mustprogress and llvm.loop.mustprogress attribute deduction.Sep 13 2020, 3:56 PM
atmnpatel edited the summary of this revision. (Show Details)
atmnpatel updated this revision to Diff 291567.Sep 14 2020, 7:11 AM

Fixed failing test.

atmnpatel updated this revision to Diff 291581.Sep 14 2020, 8:22 AM

Tests renamed.

In what situation do we generate mustprogress function attributes now? I was expecting them in clang/test/CodeGen/attr-mustprogress.cpp but did not see any.

atmnpatel updated this revision to Diff 294569.Sun, Sep 27, 3:13 PM

Split them into pre and post forward progress requirement tests, now the difference is much easier to catch.

atmnpatel added inline comments.Sun, Sep 27, 3:14 PM
clang/test/CodeGen/attr-mustprogress-0.cpp
3

changing this asap.

atmnpatel updated this revision to Diff 294570.Sun, Sep 27, 3:20 PM

All language standards (minus gnu extensions) are now tested.

atmnpatel updated this revision to Diff 294575.Sun, Sep 27, 3:52 PM

rebase to hopefully fix buildbot

jdoerfert added inline comments.Mon, Sep 28, 1:31 PM
clang/test/CodeGen/attr-mustprogress-1.cpp
25

Now here, and below with while(1) I would *not* expect mustprogress.

186

Here I would not expect the function attr but the metadata on the second loop. I guess we might want to add the metadata always for loops w/o a constant condition especially because the order of the two loops might be swapped (please add a test).

atmnpatel updated this revision to Diff 294864.Mon, Sep 28, 7:07 PM

more fixes.

jdoerfert added inline comments.Tue, Sep 29, 9:29 AM
clang/lib/CodeGen/CGStmt.cpp
802

Also in C? And C2x in the end is probably CPLusPlus2x?

892

Maybe call this FnIsMustProgress or the other one LoopMustProgress.

896

same as above

942

same as above.

Don't we have to update IsMustProgress here too?

clang/lib/CodeGen/CodeGenFunction.cpp
1166

no braces. why the mustprogress check?

1170

Say that we do this late because we need to see the body.

atmnpatel updated this revision to Diff 295119.Tue, Sep 29, 2:12 PM

NFC fixes.

atmnpatel marked 3 inline comments as done.Tue, Sep 29, 4:19 PM
atmnpatel added inline comments.
clang/lib/CodeGen/CGStmt.cpp
802

Also in C?

Not quite sure what you mean here.

But LangOptions.def seems to say that C2x is the upcoming C standard, not any upcoming C++ standard.

atmnpatel updated this revision to Diff 295149.Tue, Sep 29, 4:24 PM

Fixing buildkite build.

aqjune added inline comments.Wed, Sep 30, 3:22 PM
clang/lib/CodeGen/CGStmt.cpp
802

A silly question: does old C/C++ not guarantee that loops should make forward progress?

atmnpatel added inline comments.Wed, Sep 30, 10:29 PM
clang/lib/CodeGen/CGStmt.cpp
802

Nope, it was introduced explicitly simultaneously in C11 (6.8.5p6) and C++11 has it implicitly through [intro.progress].

One minor nit from me.

@jyknight @aaron.ballman @rjmccall any more thoughts?

clang/lib/CodeGen/CGStmt.cpp
802

Move this condition into a helper function somewhere with documentation. If we use another (similar) condition the same. This has to be updated over time and that way makes it much easier (also to read).

atmnpatel updated this revision to Diff 296752.Wed, Oct 7, 11:24 AM

Bump for bot.