Page MenuHomePhabricator

[hip] Fix HIP version parsing.
ClosedPublic

Authored by hliao on Dec 19 2020, 3:56 PM.

Details

Summary
  • Need trimming before parsing major or minor version numbers. This's required due to the different line ending on Windows.
  • In addition, the integer conversion may fail due to invalid char. Return that parsing function return true when the parsing fails.

Diff Detail

Event Timeline

hliao created this revision.Dec 19 2020, 3:56 PM
hliao requested review of this revision.Dec 19 2020, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2020, 3:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tra added inline comments.Tue, Jan 5, 12:37 PM
clang/lib/Driver/ToolChains/AMDGPU.cpp
91–93

A comment describing expected input format would be helpful here.

91–93

A comment that it returns true on failure to parse would be helpful.

98–106

Part.trim().split() (or, maybe, rtrim()if you only care about EOL) would save you trimming in each individual case.

yaxunl added a comment.Wed, Jan 6, 7:35 AM

Pls address Artem's comments. LGTM otherwise.

hliao marked an inline comment as done.Wed, Jan 6, 10:44 AM
hliao added inline comments.
clang/lib/Driver/ToolChains/AMDGPU.cpp
98–106

sure, as that file is auto-gen so far, we could simplify the unnecessary parsing so far. The EOL is the only issue so far.

hliao updated this revision to Diff 314941.Wed, Jan 6, 10:45 AM
hliao marked an inline comment as done.

Revise following reviewers' comments.

hliao marked 2 inline comments as done.Wed, Jan 6, 10:45 AM
tra accepted this revision.Wed, Jan 6, 11:23 AM

LGTM overall.

clang/lib/Driver/ToolChains/AMDGPU.cpp
92

It still does not desribe what we expect to see in that file.
E.g.:

HIP_VERSION_MAJOR=1
HIP_VERSION_MINOR=2
HIP_VERSION_PATCH=3
100

Perhaps we could use StringSwitch here:

int &Value = llvm::StringSwitch<int&>(Splits.first)
    .Case("HIP_VERSION_MAJOR", Major)
    .Case("HIP_VERSION_MINOR", Minor)
    .Case("HIP_VERSION_PATCH", VersionPatch)
if (Splits.second.getAsInteger(0, Value))
      return true;
This revision is now accepted and ready to land.Wed, Jan 6, 11:23 AM
This revision was automatically updated to reflect the committed changes.