[PATCH] D81736: [openmp] Base of tablegen generated OpenMP common declaration

Joel E. Denny via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 17 11:51:02 PDT 2020


jdenny added a comment.

This patch looks like a good idea to me.  Thanks.

In D81736#2090419 <https://reviews.llvm.org/D81736#2090419>, @clementval wrote:

> @jdoerfert Some tests in clang relies on the position of the specific enum in the declaration. TableGen is sorting the records so the enum is generated in alphabetical order therefore the enum position is changed. Is it fine to update the test with smith like `{{.*}}` instead of the specific id?
>
> Example of failure: Old position 9, new position 23
>
>   ./llvm-project/clang/test/AST/ast-dump-openmp-target-teams-distribute-simd.c:74:16: error: CHECK-NEXT: expected string not found in input
>   // CHECK-NEXT: | | | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 9
>                  ^
>   <stdin>:51:1: note: scanning from here
>   | | | | | `-OMPCaptureKindAttr 0x43ee9600 <<invalid sloc>> Implicit 23
>  
>


This seems like a case where the dumper should be printing a name instead of a number.  Given that you're touching all the tests for that anyway, I wonder how hard it would be to go ahead and fix that, perhaps in a parent patch so that this patch becomes easier to review.  Ignore this suggestion if it's too much of a distraction.



================
Comment at: clang/test/AST/ast-dump-openmp-target-parallel-for.c:105
 // CHECK-NEXT: |       | | `-FieldDecl {{.*}} <line:5:3> col:3 implicit 'int'
-// CHECK-NEXT: |       | |   `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 9
+// CHECK-NEXT: |       | |   `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 23
 // CHECK-NEXT: |       | `-CapturedDecl {{.*}} <<invalid sloc>> <invalid sloc> nothrow
----------------
Why 23 not `{{.*}}` like the others?


================
Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:22
+  //
+  // By default, uses the name of the dialect as the only namespace. To avoid
+  // placing in any namespace, use "". To specify nested namespaces, use "::"
----------------
"dialect" -> "directive language" as above?


================
Comment at: llvm/test/TableGen/directive.td:11
+  let clausePrefix = "TDLC_";
+  let makeEnumAvailableInNamespace = 1;
+}
----------------
Does it make sense to go ahead and test the case where this is 0?  What about testing enableBitmaskEnumInNamespace?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81736





More information about the llvm-commits mailing list