Page MenuHomePhabricator

[openmp][openacc] Check for duplicate clauses for directive
ClosedPublic

Authored by clementval on Tue, Oct 27, 8:28 AM.

Details

Summary

Check for duplicate clauses associated with directive. Clauses can appear only once
in the 4 lists associated with each directive (allowedClauses, allowedOnceClauses,
allowedExclusiveClauses, requiredClauses). Duplicates were already present (removed with this
patch) or were introduce in new patches by mistake (D89861).

Diff Detail

Event Timeline

clementval created this revision.Tue, Oct 27, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 27, 8:28 AM
clementval requested review of this revision.Tue, Oct 27, 8:28 AM

This is a good verification check. A few questions.

llvm/include/llvm/TableGen/DirectiveEmitter.h
106

Any issues with moving this to the base class?

llvm/test/TableGen/directive3.td
12–13

Nit: Extra line/lines?

llvm/utils/TableGen/DirectiveEmitter.cpp
119

Wouldn't this lead to an O(N^2) loop. Can we do better?

clementval added inline comments.Tue, Oct 27, 4:19 PM
llvm/include/llvm/TableGen/DirectiveEmitter.h
106

I'll move it.

llvm/utils/TableGen/DirectiveEmitter.cpp
119

CrtClauses is a StringSet implemented on the top of StringMap. The lookup should be constant (O(1)) so inside the loop we should be O(N).

clementval marked 2 inline comments as done.

Address review comment

clementval marked an inline comment as done.Tue, Oct 27, 6:09 PM
llvm/test/TableGen/directive3.td
31

Nit: Is this second error giving any additional information? Is this twin errors so that we get all occurrences of errors rather than fail at the first one?

llvm/utils/TableGen/DirectiveEmitter.cpp
119

Ahh, OK. Completely missed that.
Nit: Would using insert and checking the return value be better than using find and insert?

150–174

Is this code from 151 to 162 similar for all occurrences of HasDuplicateClausesInDirectives? If so would it be better to pull it into a function and use that function?

clementval marked an inline comment as done.

Address review comments

llvm/test/TableGen/directive3.td
31

The first error is the specific error triggered for each duplicate found. The last one is the general error mentioning that one or more duplicates have been found. I'm happy to remove the global error if it is confusing.

llvm/utils/TableGen/DirectiveEmitter.cpp
150–174

I extract it to a function. I plan to add a wrapper for Records so we can get rid of all the Records.getAllDerivedDefinitions("DirectiveLanguage"); and have simple accessor and also pass just a reference to the records to all those functions that now takes vector of Records. But that will be in a next patch.

LGTM.

llvm/utils/TableGen/DirectiveEmitter.cpp
150–174

OK

This revision is now accepted and ready to land.Wed, Oct 28, 11:48 AM
This revision was landed with ongoing or failed builds.Wed, Oct 28, 12:12 PM
This revision was automatically updated to reflect the committed changes.
Paul-C-Anagnostopoulos reopened this revision.EditedThu, Oct 29, 10:26 AM

When I run the TableGen tests on master, I get the output below. I just rebased master to be sure I am up to date.

 -- Testing: 1 tests, 1 workers --
FAIL: LLVM :: TableGen/directive3.td (1 of 1)
******************** TEST 'LLVM :: TableGen/directive3.td' FAILED ********************
Script:
--
: 'RUN: at line 1';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-decl -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
: 'RUN: at line 2';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-impl -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
: 'RUN: at line 3';   not d:\llvm\build\bin\llvm-tblgen.exe -gen-directive-gen -I C:\LLVM\llvm-project\llvm\test\TableGen/../../include C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td 2>&1 | d:\llvm\build\bin\filecheck.exe -match-full-lines C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "not" "d:\llvm\build\bin\llvm-tblgen.exe" "-gen-directive-decl" "-I" "C:\LLVM\llvm-project\llvm\test\TableGen/../../include" "C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td"
note: command had no output on stdout or stderr
error: command failed with exit status: 1
$ "d:\llvm\build\bin\filecheck.exe" "-match-full-lines" "C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td"
# command stderr:
C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td:29:11: error: CHECK: expected string not found in input
// CHECK: error: Clause TDLC_ClauseA already defined on directive TDL_DirA
          ^
<stdin>:1:1: note: scanning from here
#ifndef LLVM_TdlError_INC
^

Input file: <stdin>
Check file: C:\LLVM\llvm-project\llvm\test\TableGen\directive3.td

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: #ifndef LLVM_TdlError_INC
check:29     X~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          2: #define LLVM_TdlError_INC
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~
          3: 
check:29     ~
          4: namespace llvm {
check:29     ~~~~~~~~~~~~~~~~
          5: class StringRef;
check:29     ~~~~~~~~~~~~~~~~
          6: namespace TdlError {
check:29     ~~~~~~~~~~~~~~~~~~~~
          .
          .
          .
>>>>>>

error: command failed with exit status: 1

--

********************
********************
Failed Tests (1):
  LLVM :: TableGen/directive3.td


Testing Time: 0.31s
  Failed: 1
This revision is now accepted and ready to land.Thu, Oct 29, 10:26 AM

When I run the TableGen tests on master, I get the output below. I just rebased master to be sure I am up to date.

This is weird because windows buildbots are not triggering the error you are seeing. Did you try with a clean build? Your lit log looks like that error was not triggered and the backend generated the file anyway.

How strange. I checked out master, rebased it, and ran the test. It worked like a charm. I have no explanation.

Sorry for the hassle.

clementval closed this revision.Sat, Oct 31, 11:26 AM