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

use FNV-1a for kernel indexing instead of md5 #45

Merged
merged 13 commits into from
Apr 29, 2016

Conversation

yan-ming
Copy link
Contributor

make test

********************
Testing Time: 634.94s
********************
Failing Tests (6):
    CPPAMP :: Unit/HC/memcpy_symbol1.cpp
    CPPAMP :: Unit/HC/memcpy_symbol3.cpp
    CPPAMP :: Unit/HC/wg_size.cpp
    CPPAMP :: Unit/HSAIL/shfl_xor.cpp
    CPPAMP :: Unit/SharedLibrary/shared_library2.cpp
    CPPAMP :: Unit/SharedLibrary/shared_library3.cpp

  Expected Passes    : 663
  Expected Failures  : 25
  Unsupported Tests  : 10
  Unexpected Failures: 6

@whchung whchung merged commit 84095e1 into ROCm:develop Apr 29, 2016
@whchung
Copy link
Collaborator

whchung commented May 5, 2016

@yan-ming I was looking at failed cases in this pull request. And unfortunately it seems the following two tests are directly resulted from switching to FNV-1a and introducing 104-byte cutoff:

CPPAMP :: Unit/SharedLibrary/shared_library2.cpp
CPPAMP :: Unit/SharedLibrary/shared_library3.cpp

Those 2 tests have 2 nearly-identical kernels with a very small difference in the names. And that gives 104 heuristic fail. I've tested they would pass if enlarge the cutoff size to 256 bytes.

@yan-ming
Copy link
Contributor Author

yan-ming commented May 5, 2016

Hi @whchung , thanks for prompting this issue up. I agree there would be collisions when two nearly identical kernels are used at a time. Changing the cutoff size to 256 doesn't make them pass on my end. It looks like the proper size here is about 270, I think it might be safer if we have like 512 in this case?

@whchung
Copy link
Collaborator

whchung commented May 5, 2016

@yan-ming you are right, I accidentally used MD5 in my previous experiment. 512 is a safer value. I've pushed a commit already.

@whchung
Copy link
Collaborator

whchung commented May 5, 2016

@yan-ming further tests show we need to use 768 so both HSAIL backend and LC backend can work.

@yan-ming
Copy link
Contributor Author

yan-ming commented May 5, 2016

@whchung could you briefly explain how did you discover this value?

@whchung
Copy link
Collaborator

whchung commented May 5, 2016

@yan-ming I made two builds of hcc, one with HSAIL backend (default one), and another with LC backend. Then run the 2 tests aforementioned. And gradually increase the cutoff value by 128 bytes.

This should be just a temporary thing. We'll push people working on LC backend and HSAIL finalizer to calculate hash and put into headers so we don't need to do them in HCC runtime anymore.

@yan-ming
Copy link
Contributor Author

yan-ming commented May 5, 2016

@whchung I see. I only use the LC backend for testing hcfft and the porting applications. Even 104 works for my cases.

I'm okay if we enlarge the cutoff size here, I agree with that the ultimate solution should be done in the finalizer.

Sign up for free to 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.

3 participants