[ROCm] Fixes for broken --config=rocm build, post PR #26722

[deven-amd]

This is a follow up to PR #26722 .

There were a couple of changes introduced in that PR that break the --config=rocm build, and this PR has fixes for the same.

1. changing if_cuda to if_cuda_is_configured in cases where a "duplicate dependency" is introduced, due to a common dependency in the CUDA + ROCm paths

2. correcting a filename typo for @rocprim_archive//:LICENSE.txt that was leading to compile error.

@tatianashp , just FYI.

@gunan , @chsigg, @whchung

Link to discussion regarding if_cuda vs if_cuda_is_configured from another PR : #26753 (comment)

[gunan]

This does not work internally.
I am not sure what the difference between if_cuda and if_cuda_is_configured is.
But to be able to merge this, someone will need to take a look into this and maybe even change the macros internally.
CC @chsigg @ezhulenev @jlebar

[deven-amd]

hi @gunan,

If possible, can you please shed some light on the nature of the problems introduced by this change. We would like to understand the problem(s) associated with this change, to see if there is anything we can do on our end to avoid/workaround them.

thanks

deven

[gunan]

Internally, there is no configure script. So if_cuda_is_configured semantically does not make sense for us internally.

Also, I am trying to understand what the differences between if_cuda and if_cuda_is_configured are. We are trying to move to a state where we do not have to run configure script, so only if_cuda will make sense in such a case.

[deven-amd]

See the discussion between @chsigg and myself here for some info on the differences between if_cuda and if_cuda_is_configured : #26753 (comment)

Internally, there is no configure script.

Do you still need to specify the env var TF_NEED_CUDA=1 to build TF with CUDA support?

• If that is the case then if_cuda_is_configured should still work.
• Else if there is some different mechanism to specify building TF with CUDA support, then we should update this routine (https://github.com/tensorflow/tensorflow/blob/master/third_party/gpus/cuda_configure.bzl#L337) to trigger using that mechanism in addition to the env var. That should make the if_cuda_is_configured work without running the configure script.

@chsigg please confirm the accuracy of above. thanks.

[gunan]

Do you still need to specify the env var TF_NEED_CUDA=1 to build TF with CUDA support?

No, we do not. Even in opensource, we would like to get to a state where all you need to do is bazel build --config=cuda //tensorflow/...

Internally, and soon externally, all you need to do for building with CUDA support is add --config=cuda flag. That is all that is available to bazel to decide what to use when building.

[deven-amd]

So for the purpose of this PR, either someone internal will need to take a look into this to make it work, or we need to find a way to make your code work with if_cuda. Unfortunately, I do not have cycles to investigate this.

Due to the nature of the error, it does not seem that this is an issue we can solve by changing things exclusively on the ROCm side of the fence..we are going to need help. Given that, and also that this is (one of the) blocking issues preventing us from filing further PRs (for upstreaming ROCm support), we need to resolve this PR ASAP. What are our options towards that end?

@chsigg, your suggestion of if_gpu(cuda_arg, rocm_arg, defaut_arg) would be one potential solution here. How would we go about implementing + testing it out?

thanks again

[chsigg]

I think it makes most sense if we clean this up from our end. I agree with Gunhan that if_cuda_is_configured (and .._compat) should go away. As far as I understand that macro was introduced to allow linking against CUDA libraries (say, cuDNN) while not invoking nvcc to build CUDA code. I'm not sure this is useful, but probably it won't be trivial to unify them again more than two years later. I will take a look and report back.

[deven-amd]

@chsigg @gunan, let me know if / how I can help develop+test the new solution, will be more than happy to help out.

In the meantime, i will push out a commit soon, that works around this issue by introducing a new if_cuda_or_rocm function. This function works the same as if_cuda / if_rocm, i.e. it will return the if_true arg when either CUDA or ROCM is enabled. Using this function to specify the common dependencies, works around the bazel error. However, this solution is less than ideal because it needs to duplicate the logic within if_cuda / if_rocm. Please review the commit and let me know if this is acceptable as a temporary workaround, while we work on the proper fix.

[chsigg]

This looks good as a temporary solution, thanks. I will run our internal tests and try to merge it.

[deven-amd]

@chsigg thank you!

I have pushed out a rebase to keep just the last commit and sync up to the tip.

[deven-amd]

@chsigg. thank you for getting the workaround merged...it will help us make progress with upstreaming our code. Let me know if / when you want us to try out the actual solution once it becomes available. There are quite a few uses of if_rocm_is_configured and I would like to remove them all, given that the _is_configured variant will be deprecated sooner or later.

don't know why this PR is still showing as "Open". Assuming you will take care of that part.

thanks again

deven

[chsigg]

Thanks Deven, I'm working on simplifying the if_cuda/rocm* situation. My current plan:

• if_*_is_configured() should refer to whether the gpu libraries are available. Searching for CUDA libraries will be enabled through --config=using_cuda, which has the effect of --define=using_cuda=true and --action_env=TF_NEED_CUDA=1. The former is the standard way to pick among config_settings, and I would like if_cuda_is_configured() to return a select statement (to match the internal implementation, which returns a select distinguishing between platforms where CUDA is available). The latter is the only way to affect a repository_rule. ROCm should have an equivalent setup, and I want to add an if_gpu_is_configured(if_cuda, if_rocm, otherwise) macro.

• if_*() should refer to whether we build op kernels gpus (this excludes XLA, which should only rely on the mechanics above). CUDA op kernels are enabled through --config=cuda, which again triggers a define (--define=using_cuda_nvcc=true or --define=using_cuda_clang=true) which triggers a config_setting which is used in a select statement. This is already the case today. Again, same for ROCm and add an if_gpu(if_cuda, if_rocm, otherwise).

Not sure why the PR still shows as open, I'm closing it manually.