This is an archive of the discontinued LLVM Phabricator instance.

[test, InferFunctionAttrs] Fix use of var defined in CHECK-NOT
ClosedPublic

Authored by thopre on Mar 30 2021, 6:38 AM.

Details

Summary

LLVM test Transforms/InferFunctionAttrs/annotate contains two RUN
invokation (UNKNOWN and NVPTX lines) which involve a CHECK-NOT directive
with a variable not defined by the enabled CHECK prefixes. This commit
fixes that by:

  • enabling CHECK prefix for unknown target with specialisation when it differs from other targets
  • checking for absence of bcmp with any attribute for NVPTX

Diff Detail

Event Timeline

thopre requested review of this revision.Mar 30 2021, 6:38 AM
thopre created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 6:38 AM
tra added a subscriber: tra.Mar 30 2021, 9:27 AM
tra added inline comments.
llvm/test/Transforms/InferFunctionAttrs/annotate.ll
241

I don't think this test does what it's supposed to do for nvptx.

The CHECK-NVPTX: attributes will match at the end of the IR and this check will be looking for the function declarations way past the point where they were, so it will never trigger, even if NVPTX does declare bcmp with the wrong arguments.

You need to move the CHECK-NVPTX: attributes from line 9 to the end of this file.

thopre updated this revision to Diff 334231.Mar 30 2021, 11:01 AM
thopre marked an inline comment as done.

Move attribute check after the CHECK-NOT, test still passes

tra added a comment.Mar 30 2021, 11:41 AM

LGTM for NVPTX.

llvm/test/Transforms/InferFunctionAttrs/annotate.ll
368

Is the goal to make sure that there are no attributes assigned? Or that we don't care if there are any attributes?
If that's the former, then the check does not work -- it will happily match even if the declaration is followed by an attribute.
If that's the latter, the check is a bit confusing as the 'don't care' part is not obvious.

Perhaps it would be better to restructure the checks this way:

; CHECK-LABEL:  declare i32 @ffsl(i64) 
; CHECK-KNOWN-SAME:  [[NOFREE_NOUNWIND_WILLRETURN]]
; if we want to make sure there are no attributes
; CHECK-UNKNOWN-NOT: #{{.*}}
;
; if we don't care about the attributes, then we could do 
; CHECK-UNKNOWN-SAME: {{.*}}
; or CHECK-UNKNOWN could be skipped altogether.

This way it's somewhat easier to tell the intent of the test.

thopre added inline comments.Mar 30 2021, 11:49 AM
llvm/test/Transforms/InferFunctionAttrs/annotate.ll
368

I'm not sure what's the expectation. All I know is the test fails with a FileCheck patch to error out on CHECK-NOT with undefined variable. There's a CHECK-UNKNOWN on line 242 that uses a variable defined by a CHECK and looking through git history the UNKNOWN case used to have no prefix (and thus used CHECK by default) and a check-prefix=CHECK-UNKNOWN was added later. My feeling is the author of the change did not realize it would stop looking at CHECK and thus I added CHECK as a prefix. That then failed for the few lines that are changed in this patch because they have no attribute. I do not know if it's expected or not, I was hoping reviewers would be able to tell me.

Note that NVPTX also does not run CHECK line by default and maybe that's also a mistake. However many functions have no attribute for NVPTX. enough that I started wondering if the lack of CHECK prefix for NVPTX might be intentional.

thopre added inline comments.Mar 31 2021, 3:45 AM
llvm/test/Transforms/InferFunctionAttrs/annotate.ll
368

Hold on, found lots of other issues with this test. I'll make a new update soon.

thopre updated this revision to Diff 334406.Mar 31 2021, 4:26 AM

Fix unchecked and duplicated attributes

llvm/test/Transforms/InferFunctionAttrs/annotate.ll
368

UNKNOWN was added in https://reviews.llvm.org/rG8e16d73346f8091461319a7dfc4ddd18eedcff13 only to check bcmp but later https://reviews.llvm.org/rG20e989e9de6abcf9a684978a2688acc4ea01036f and other commits added more checks for memory functions. Clearly the intent is for all functions to be checked with UNKNOWN. I've adapted the checks according to your suggestion for absence of attribute.

NVPTX was added with https://reviews.llvm.org/rGae272d718e882b18258a587ce111a4150d761b5e

It looks like it was specifically intended to check only for __nvvm_reflect. Then the CHECK-NVPTX-NOT was added in https://reviews.llvm.org/rG8e16d73346f8091461319a7dfc4ddd18eedcff13

With the bcmp check for non linux now using {{$}} to check absence of attribute, NVPTX RUN line now only need CHECK-NVPTX.

tra accepted this revision.Mar 31 2021, 10:36 AM

LGTM with a minor nit.

llvm/test/Transforms/InferFunctionAttrs/annotate.ll
1032–1033

These should probably be -DAG, too.

This revision is now accepted and ready to land.Mar 31 2021, 10:36 AM
This revision was automatically updated to reflect the committed changes.
thopre marked 2 inline comments as done.