LGTM

In D154280#4519029, @xgupta wrote:

Hi,

writing test case in llvm/utils/lit/tests seems difficult, as when we run them, there is no .script file generated as in case with other llvm or clang test cases. So the patch is not working there and the env variable is not setting. I am thinking if we can submit in this way without testcase since lit code is very stable so chances of breaking something is less.

shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ bin/llvm-lit -a -v --per-test-coverage  ../clang/test/SemaCXX/bool.cpp

llvm-lit: /home/shivam/.llvm/llvm-upstream/llvm/utils/lit/lit/llvm/config.py:484: note: using clang: /home/shivam/.llvm/llvm-upstream/build/bin/clang
-- Testing: 1 tests, 1 workers --
PASS: Clang :: SemaCXX/bool.cpp (1 of 1)
Script:
--
export LLVM_PROFILE_FILE=bool0.profraw && : 'RUN: at line 1';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool1.profraw && : 'RUN: at line 2';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++14 -fsyntax-only -verify=expected,precxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
export LLVM_PROFILE_FILE=bool2.profraw && : 'RUN: at line 3';   /home/shivam/.llvm/llvm-upstream/build/bin/clang -cc1 -internal-isystem /home/shivam/.llvm/llvm-upstream/build/lib/clang/17/include -nostdsysteminc -std=c++2b -fsyntax-only -verify=expected,cxx17 -Wno-constant-conversion -Wno-deprecated -Wdeprecated-increment-bool /home/shivam/.llvm/llvm-upstream/clang/test/SemaCXX/bool.cpp
--
Exit Code: 0


********************

Testing Time: 0.76s
  Passed: 1
shivam@linux ~/.llvm/llvm-upstream/build (arcpatch-D154280) $ find . -type f -name "*.profraw" | grep -i "profraw"

./default.profraw
./tools/clang/test/SemaCXX/bool1.profraw
./tools/clang/test/SemaCXX/bool0.profraw
./tools/clang/test/SemaCXX/bool2.profraw

In D154520#4474216, @abrachet wrote:

That works. Thanks a lot, @hnrklssn

@abrachet Created D154520. Can you verify that it works for you?

@nemanjai @TWeaver It should be fixed by 4488ee2. Are the buildbots still failing with that commit?

Handle lack of git URL, as well as custom additions to the version message.

This patch has already been landed as 6d64faf. This is my first time requesting a post-commit review (since I broke the buildbot and the fix was trivial), so please let me know if I missed something in that process.

Update test cases after rebasing on ToT

Add unnamed global test case

@nikic Are you happy with the current patchset?

Please also add a test of lit itself (see llvm/utils/lit/tests). I think something using --emit-coverage on a test that echoes $LLVM_PROFILE_FILE should be enough.

@nikic Ping :)

In D148216#4434171, @nikic wrote:

There are some test failures.

I believe there is one bug with the handling of unnamed globals. Previously we produced this:

; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals --version 2
; RUN: opt -S < %s | FileCheck %s
@0 = global i32 0

;.
; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
;.
define i32 @test() {
; CHECK-LABEL: define i32 @test() {
; CHECK-NEXT:    [[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
; CHECK-NEXT:    ret i32 [[V]]
;
  %v = load i32, ptr @0
  ret i32 %v
}

And now we instead check CHECK: @0 = global i32 0, so there is a mismatch between definition and use. I believe it does make sense to keep the wildcard names for unnamed globals.

On the same test case, if I keep rerunning update_test_checks on the same file, it keeps adding extra CHECK: @0 = global i32 0 lines at the start (before the first ;.. That didn't happen previously.

Added back regex matchers for anonymous global values

Keep supporting --check-globals without explicit level as meaning '--check-globals all'

In D148216#4431456, @jdoerfert wrote:

could you put a little more information in the commit message please. "It won't do X when we do Y", could mean a lot of things. We don't do Y anymore, or we do X' now, with various choices for X'.

Rebase after reformatting with Black, update commit message

@nikic Ping :)

@nikic Did you have a look at the new patch?

Revert checker behaviour for global variables to check hard-coded identifiers
Remove 'seen' mode
Replace 'transitive' mode with 'smart', where attributes are no longer checked as metadata
Introduce version bump to v3, and make 'smart' mode the default in v3

Overhauled how update_cc_tests.py checks globals

In D148216#4266201, @nikic wrote:

We should probably just use the META fallback for all metadata not already explicitly handled. I think there's pretty little value in adding a special name for each metadata kind.

In D148216#4266174, @arichardson wrote:

Change looks good to me, but if clang can't emit these yet we should probably wait until committing this? Do you have a link to a review that uses this metadata?

Add test case emitting !annotation from clang.
Make annotation matching a generic metadata fallback.

I'm not familiar with any upstream ways to trigger !annotations so I unfortunately don't have any test updates to upstream at this time. I'm open to suggestions for adding ways to test the change though.

Add explicit labels to FileCheck checks

Made changes in line with what @fcloutier suggested.

No longer reorders annotations to occur before codegen. Instead EmitTrapCheck checks the function declaration for OptimizeNoneAttr.

Ran clang-format

Rebased onto upstream/main

Forgot to rebase onto upstream... That explains why it went so smoothly!

Rebasing to master

Cool :) This is my first submission, so I don't have commit access. If I understand correctly that means I need someone else to commit it on my behalf, yes?

Fixed formatting issue

Split CHECKs to keep them closer to their relevant functions, and added debug information to disambiguate instructions where relevant.

It should be ready to review again now :)

Removed undefs in test cases.

After a long wait I'm now finally able to work on this again! Please excuse the delay.

Addressed review comments and fixed a crash for non-local variables.

In D79097#2218587, @anemet wrote:

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Updating D79564: [Loop-Vectorize] update test cases to accept new ouput

Updating D79097: reformatting code according to pre-merge check

Accidentally created an extra revision

Updating D79097: [GVN] Improve analysis for missed optimization remark

[GVN] [Remarks] Trying to push missing file

This is my first time using both Phabricator and Arcanist, so I'm not entirely familiar with the UI and workflow, but I can't seem to find one of the test cases I added in my update. It's supposed to be in llvm/test/Transforms/GVN/opt-remarks-multiple-users.ll, but I can't see it added in the revision. Did I miss something? It's an earlier commit than the new diff I pushed, so I would've thought it would be included as well.

[GVN] Update analysis for optimization remark and add tests