[PATCH] D99589: [test, InferFunctionAttrs] Fix use of var defined in CHECK-NOT

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/annotate.ll:370
+; CHECK-KNOWN: declare i32 @ffsl(i64) [[NOFREE_NOUNWIND_WILLRETURN]]
+; CHECK-UNKNOWN: declare i32 @ffsl(i64)
 declare i32 @ffsl(i64)
----------------
thopre wrote:
> thopre wrote:
> > tra wrote:
> > > 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.
> > 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.
> Hold on, found lots of other issues with this test. I'll make a new update soon.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99589/new/

https://reviews.llvm.org/D99589