Skip to content
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

Resolve failures on SLES #2002

Merged
merged 6 commits into from
Aug 12, 2024
Merged

Resolve failures on SLES #2002

merged 6 commits into from
Aug 12, 2024

Conversation

ellosel
Copy link
Contributor

@ellosel ellosel commented Aug 7, 2024

These changes solve test failures in SLES containers when running TensileTests.

Fixes

  1. The decision tree test fix is a bug (out of bounds error) in the test code that isn't caught on other platforms.
  2. The change in Loading.cpp was necessary to avoid another segfault when running getFile. The other function achieves the same outcome but does not depend on boost optional which seems to cause a segfault when initializing in the context of this code.

@ellosel ellosel added the NoCI Don't run CI label Aug 7, 2024
@ellosel ellosel requested a review from bstefanuk August 7, 2024 18:59
@ellosel ellosel self-assigned this Aug 7, 2024
@nakajee
Copy link
Contributor

nakajee commented Aug 7, 2024

Please always update year in Copyright if it is not 2024.

@@ -369,6 +369,8 @@ TEST(DecisionTree, DecisionTreeBatch)
using SizeInRange = Predicates::Contraction::SizeInRange;
using Range = Predicates::Contraction::Range;
using And = Predicates::And<ContractionProblem>;
using Key = std::array<float, 4>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how Key works, but Key is used at line 374. Isn't it a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dimension of key should be greater than or equal to the number of features (or features.size() (however, maybe someone more familiar with solution selection could confirm).

The last test was added along with the additional feature but the Key was reused from previous tests where the size was 3. This caused a segfault which is unsuprising because we are trying to access the fourth element of a size three array. What may be a surprise is that this is only an issue on SLES. However, this is UB and the compiler is free to do what it wants. It is possible that this code was optimized out on the other platforms.

As for line 342, we are shadowing the Key definition that is at the top of the file. Perhaps we should move these aliases into each of the test cases.

@ellosel ellosel removed the NoCI Don't run CI label Aug 7, 2024
@ellosel ellosel marked this pull request as ready for review August 8, 2024 16:05
@ellosel
Copy link
Contributor Author

ellosel commented Aug 8, 2024

@nakajee @bstefanuk please resolve comments.

@nakajee
Copy link
Contributor

nakajee commented Aug 8, 2024

Thanks for your update.
I do not have further comments for code change.
I am not sure how to confirm if this change works or not.
You ran some local test and confirmed this change worked, right?

@ellosel
Copy link
Contributor Author

ellosel commented Aug 8, 2024

Thanks for your update. I do not have further comments for code change. I am not sure how to confirm if this change works or not. You ran some local test and confirmed this change worked, right?

In addition to local testing I ran in a manual pipeline and everything looks good. Once this is merged I will enable sles testing.

Copy link
Contributor

@nakajee nakajee left a comment

Choose a reason for hiding this comment

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

Thanks for your comment.
Looks good.

@ellosel ellosel merged commit 3b7173f into ROCm:develop Aug 12, 2024
16 checks passed
ellosel added a commit to ellosel/Tensile that referenced this pull request Sep 11, 2024
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.

3 participants