Skip to content

[Target][Codegen] Use target class in all codegens - #6347

Merged
zhiics merged 6 commits into
apache:masterfrom
junrushao:feature/2020-08-26/target-export-codegen
Aug 29, 2020
Merged

[Target][Codegen] Use target class in all codegens#6347
zhiics merged 6 commits into
apache:masterfrom
junrushao:feature/2020-08-26/target-export-codegen

Conversation

@junrushao

@junrushao junrushao commented Aug 27, 2020

Copy link
Copy Markdown
Member

This PR uses target class to replace almost all the raw target strings in all the codegen modules. It further helps with us migration towards a robost JSON-like targets configuration, per [RFC] TVM Target Specification.

The only place that uses raw string is what is stored in tvm_target in LLVM module's metadata. In this case, we know that we only care about "mtriple", "mattr", "mcpu", "mfloat-abi", so we only store those attributes in the metadata.

CC: @comaniac @jwfromm @jroesch @tqchen

@comaniac comaniac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also cc @zhiics @yzhliu @icemelon9

Comment thread src/target/llvm/codegen_amdgpu.cc Outdated
Comment thread src/target/llvm/codegen_nvptx.cc Outdated
Comment thread src/target/llvm/codegen_nvptx.cc Outdated
Comment thread src/target/llvm/llvm_common.cc Outdated
Comment thread src/target/llvm/llvm_common.cc Outdated
Comment thread src/target/llvm/llvm_module.cc Outdated

@zhiics zhiics left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@comaniac comaniac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@junrushao

Copy link
Copy Markdown
Member Author

The CI is green. Could you guys take another look? Thanks! @zhiics @comaniac

@zhiics
zhiics merged commit d9450f8 into apache:master Aug 29, 2020
@zhiics

zhiics commented Aug 29, 2020

Copy link
Copy Markdown
Member

Thanks @junrushao1994 @comaniac

@t-vi

t-vi commented Sep 10, 2020

Copy link
Copy Markdown
Contributor

The ROCm default detection seems to have been mangled to confuse ROCm version (software) with compute arch (hardware, e.g. gfx). I'll try to fix it.

@junrushao

Copy link
Copy Markdown
Member Author

@t-vi Yes, the detection logic used previously in amdgpu codegen is here: https://github.com/apache/incubator-tvm/blob/e5b793f39fd5b4f84b0aedf06aa376ebe45cf2bc/src/target/llvm/codegen_amdgpu.cc#L194. Then I moved the logic to the target constructor to reveal it at earliest stage.

@junrushao

junrushao commented Sep 10, 2020

Copy link
Copy Markdown
Member Author

@t-vi I see. We should change this line: https://github.com/apache/incubator-tvm/blob/master/src/target/target_kind.cc#L176, from runtime::kApiVersion to runtime::kGcnArch . Is that correct?

@t-vi

t-vi commented Sep 10, 2020

Copy link
Copy Markdown
Contributor

I'll just send a PR in a minute or so.

kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 17, 2020
* [Target][Codegen] Make all code generator use Target class instead of target string

* Remove dep to TargetNode::str() in LLVM module

* Allow  for llvm nvptx codegen

* ...

* Address comments from Cody

* Rename UpdateTargetConfig => UpdateTargetConfigKeyValueEntry
kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 18, 2020
* [Target][Codegen] Make all code generator use Target class instead of target string

* Remove dep to TargetNode::str() in LLVM module

* Allow  for llvm nvptx codegen

* ...

* Address comments from Cody

* Rename UpdateTargetConfig => UpdateTargetConfigKeyValueEntry
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 18, 2020
* [Target][Codegen] Make all code generator use Target class instead of target string

* Remove dep to TargetNode::str() in LLVM module

* Allow  for llvm nvptx codegen

* ...

* Address comments from Cody

* Rename UpdateTargetConfig => UpdateTargetConfigKeyValueEntry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants