-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix literal rounding in codegen functions #1910
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1910 +/- ##
========================================
Coverage 91.37% 91.37%
========================================
Files 419 419
Lines 15544 15544
========================================
Hits 14204 14204
Misses 1340 1340
|
test/gpu/codegen_literal.cpp
Outdated
#include "migraphx/op/mul.hpp" | ||
#include <iostream> | ||
#include <vector> | ||
#include <migraphx/operators.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont include this header.
test/gpu/codegen_literal.cpp
Outdated
#include <migraphx/program.hpp> | ||
#include <migraphx/register_target.hpp> | ||
#include <migraphx/verify.hpp> | ||
#include <migraphx/pass_manager.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? I dont see it using passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, going to clean up and remove unneeded headers
test/gpu/codegen_literal.cpp
Outdated
// and after rounding must equal 63, not 64. | ||
TEST_CASE(mul_literal_round_test) | ||
{ | ||
auto run_prog = [](migraphx::program p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a free function.
test/gpu/codegen_literal.cpp
Outdated
result.visit([&](auto v) { res.assign(v.begin(), v.end()); }); | ||
}; | ||
|
||
auto create_program = [&] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline this.
test/gpu/codegen_literal.cpp
Outdated
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
* THE SOFTWARE. | ||
*/ | ||
#include "migraphx/op/mul.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont include this header.
Is it possible to add a TEST_CASE(literal_to_string_float_precision)
{
migraphx::literal x{1 / 0.00787402f};
EXPECT(x.to_string() == "126.99993142003703");
} This can probably go in |
Seems like this test fails because the string value doesn't match directly. I think I will rewrite it so it checks that the number doesn't round up. |
Check results before merge 🔆 |
🔴cadene-dpn92_1: FAILED: MIGraphX is not within tolerance - check verbose output |
// and after rounding must equal 63, not 64. | ||
TEST_CASE(mul_literal_round_test) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line
{ | ||
if(m_in.count(x.first) > 0) | ||
{ | ||
m[x.first] = t.copy_to(m_in[x.first]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can call to set_offload_copy
to do this.
Fixes the failing test case in #1815. Added a test that would otherwise fail without the change.