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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99589