Skip to content

[Relay] Support i16, f16 scalars in Relay text - #11224

Merged
Mousius merged 7 commits into
apache:mainfrom
mbs-octoml:mbs-collage-scalars
May 19, 2022
Merged

[Relay] Support i16, f16 scalars in Relay text#11224
Mousius merged 7 commits into
apache:mainfrom
mbs-octoml:mbs-collage-scalars

Conversation

@mbs-octoml

@mbs-octomlmbs-octoml commented May 5, 2022

Copy link
Copy Markdown
Contributor

While testing fp16 models for Collage discovered the Relay text
format did not support f16. While adding that cleaned up scalar handling
in general. However I left two inlined tests for 'is simple const'
in place (fuse_ops.cc and memory_alloc.cc) since it's not clear whether
they should remain specific to just {i,f}{32,64} or whether they can
be replaced with the support::IsSimpleScalar central predicate.

Also include spans in pretty printed form since I've started populating
them when debugging Collage.

@mbs-octoml
mbs-octomlforce-pushed the mbs-collage-scalars branch from d58d5f1 to c655b0fCompareMay 6, 2022 20:09

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

I think this is generally an improvement, but I'm curious whether we could cover more of the error scenarios in test cases? There's three different diagnostic errors for both int and float plus the clipping errors - though I'm unsure how easy it is to trigger them with current testing infrastructure.

Comment threadtests/python/relay/test_target_hooks.py Outdated
Comment threadtests/python/relay/test_ir_parser.py
Comment threadtests/python/relay/test_ir_text_printer.py
While testing fp16 models for Collage discovered the Relay text
format did not support f16. While adding that cleaned up scalar handling
in general. However I left two inlined tests for 'is simple const'
in place (fuse_ops.cc and memory_alloc.cc) since it's not clear whether
they should remain specific to just {i,f}{32,64} or whether they can
be replaced with the support::IsSimpleScalar central predicate.
@mbs-octoml

mbs-octoml commented May 18, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks @Mousius (and sorry for the delay, been off an a separate adventure). I think the range checking testing is ok, and I added range checks for fp16 too. For the LOG(FATALS) I added cc tests since it's tricky to tickle the failures via python. PTAL.

@mbs-octoml
mbs-octomlforce-pushed the mbs-collage-scalars branch from 088671f to 86d3434CompareMay 19, 2022 00:19
@Mousius
Mousius merged commit 534c38b into apache:mainMay 19, 2022
@Mousius

Copy link
Copy Markdown
Member

Thanks @mbs-octoml 😸 nice clean up!

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.

2 participants

@mbs-octoml@Mousius