[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99589