[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr - #16523

Merged
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2
Feb 20, 2024
Merged

[SVE] Change the dtype of Ramp and Broadcast lanes to PrimExpr#16523
lhutton1 merged 4 commits into
apache:mainfrom
ekalda:p2-scalable-ramps2

Conversation

@ekalda

Copy link
Copy Markdown
Contributor

This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.

vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())

We will use negative values for runtime::DataType the encode the scalable lane values, e.g. the above example would result in lanes = -4. That's because the lanes in runtime::DataType are tied to DLPack standard which uses uint16_t for the lanes. The conversion happens in the node definitions and runtime::DataType, so the int and uint16_t values should never be exposed to the API user, especially after the string support has been added.

Also include the TVMScript support for scalable Ramp and Broadcasts.

Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for data type string representation or LoopVectorizer support. All of these will be part of future patches.

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@ekalda

Copy link
Copy Markdown
ContributorAuthor

@tvm-bot rerun

@lhutton1lhutton1 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.

Thanks for the great work @ekalda! I had a look over and noticed a few nitpicks :)

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/arith/rewrite_simplify.cc Outdated
Comment threadsrc/target/spirv/codegen_spirv.cc Outdated
Comment threadsrc/arith/int_set.cc
Comment threadsrc/arith/pattern_match.h
Comment threadsrc/arith/rewrite_simplify.h Outdated
@tqchen

Copy link
Copy Markdown
Member

if it is not high effort, consider add https://github.com/apache/tvm/blob/main/python/tvm/ir/json_compact.py so previously serialized node can be loaded

Comment threadsrc/arith/scalable_expression.cc Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
Comment threadinclude/tvm/runtime/data_type.h Outdated
*/
inline int GetVectorBytes(DataType dtype) {
if (dtype.is_scalable()) {
LOG(FATAL) << "Cannot get vector bytes of scalable vector";

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.

runtime Data type cannot be scalable vector

Comment threadinclude/tvm/runtime/data_type.h
@ekalda

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen, @Lunderberg and @lhutton1 for your feedback, I uploaded a reworked version of the patch. Here's what's changed:

  • Separation of vscale multiplier and fixed length vector lanes APIs in runtime::DataType - now we access these constants via vscale_factor() and lanes() methods
  • is_vector() is retierd now and replaced with is_scalable_vector(), is_fixed_length_vector() and is_scalable_or_fixed_length_vector()
  • Refactor of the function that extracts the integer multiplier from a lanes expression as per @Lunderberg's suggestions
  • Removed the ScalableLanes function. Actually, the new form of ExtractVscaleFactor does the job there, so I used that. I reckon that
    if (!arith::ExtractVscaleFactor(lanes.Eval())):
    ... 
    reads somewhat weird, so LMK if you think it would be better to wrap it into something more self-documenting.
  • Now that an attempt to fetch lanes() on a scalable vector results in an error, I decided to change the pattern in codegens
    ICHECK(!op->dtype.is_scalable()) << "Scalable vectors are not supported in codegen_c_host";
    int lanes = static_cast<int>(Downcast<IntImm>(op->lanes)->value);
    
    into
    int lanes = op->dtype.lanes();
    
    Which essentially pushes the error into runtime::Datatype. This has the advantage of reducing the logic in codegens that is not really related to these codegens.
  • Implemented JSON serialisation support such that graphs serialised with older versions of TVM can be correctly loaded in versions that include the changes is this patch. Unfortunately, in case of a strategic choice of lanes value, a graph serialised with an older version of TVM can be loaded as an incorrect graph without triggrering an error that would then trigger the upgrade_json function, so now we have to force the json upgrade every time we try to load a serialised graph.

ekaldaand others added 4 commits February 14, 2024 09:57
…imExpr
This change will allow us to express scalable vectors through Ramp and Broadcast nodes, e.g.
```
vec = tvm.tir.expr.Ramp(0, 1, 4 * tvm.tir.vscale())
```
We will use negative values for `runtime::DataType` the encode the scalable lane values, e.g.
the above example would result in `lanes` = -4. That's because the lanes in `runtime::DataType`
are tied to DLPack standard which uses `uint16_t` for the lanes. The conversion happens in the
node definitions and `runtime::DataType`, so the `int` and `uint16_t` values should never be
exposed to the API user, especially after the string support has been added.
Also include the TVMScript support for scalable Ramp and Broadcasts.
Note that this patch doesn't include lowering to the appropriate LLVM vectors, support for
data type string representation or `LoopVectorizer` support. All of these will be part of
future patches.
Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Co-authored-by: Neil Hickey <neil.hickey@arm.com>
Change-Id: I8eb77ce5632359b6e4a2e63c4da490e3abab3ee9
* Separate APIs for fixed length and scalable vectors
* Improve the function that extracts the multiplier from scalable lanes
expression
* Update json_compact.py
Fix failures in test_arith_intset.py and test_tvmscript_printer_tir.py
@lhutton1
lhutton1 merged commit a6157a6 into apache:mainFeb 20, 2024
@lhutton1

Copy link
Copy Markdown
Contributor

Thanks @ekalda@tqchen@Lunderberg

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ekalda@tqchen@lhutton1@Lunderberg