[libclc] Skip opt command if opt_flags is empty

[wenju-he]

When the flag is empty, the opt command probably won't modify the bitcode; however, the command is slow for large bitcode files in debug mode.

[wenju-he]

@frasercrmck could you please review? thanks

[frasercrmck]

I think this makes sense. I can't say for sure that opt does absolutely nothing when passed no arguments but logically it makes sense.

As for how targets are organized, I wonder if conditionally providing targets adds more complexity, both for the codebase and for developers as users. Another option would be to unconditionally have builtin.opt targets but if no flags are passed, just make them empty targets that rely only on builtin.link targets. That way developers can consistently build builtin.opt targets without wondering why it says "no such target" or whatever. Just a thought.

[wenju-he]

Another option would be to unconditionally have builtin.opt targets but if no flags are passed, just make them empty targets that rely only on builtin.link targets.

Thanks for the suggestion. Done. Please review again.

[frasercrmck]

LGTM

[wenju-he]

@frasercrmck please help to merge, thanks. I don't have merge access.

[frasercrmck]

I just realised this code isn't triggered upstream. I take it you have a downstream making use of it?

I tried it locally and this needs to be if( "${ARG_OPT_FLAGS} STREQUAL "" ) or maybe better yet if ( NOT ARG_OPT_FLAGS ). The code as-is generates an error if I pass in an empty OPT_FLAGS:

CMake Error at /llvm-project/libclc/cmake/modules/AddLibclc.cmake:345 (if):
  if given arguments:

    "STREQUAL" ""

  Unknown arguments specified

[wenju-he]

I just realised this code isn't triggered upstream. I take it you have a downstream making use of it?

there is a case in upstream LLVM that opt flag is empty:

llvm-project/libclc/CMakeLists.txt

Line 353 in ad5cac3

set( opt_flags )

I tried it locally and this needs to be if( "${ARG_OPT_FLAGS} STREQUAL "" ) or maybe better yet if ( NOT ARG_OPT_FLAGS ). The code as-is generates an error if I pass in an empty OPT_FLAGS:

CMake Error at /llvm-project/libclc/cmake/modules/AddLibclc.cmake:345 (if):
  if given arguments:

    "STREQUAL" ""

  Unknown arguments specified

Thanks for the testing. Changed to if ( NOT ARG_OPT_FLAGS )
I don't know what happened, a few days ago I did observe that bitcode size become smaller or larger when opt flag is empty.

[wenju-he]

I remembered. I was testing for amdgpu arch. I just switched the code between if and else blocks for testing, and didn't set opt_flags to empty. So the check was wrong at the beginning.

[frasercrmck]

That's true about SPIR-V, but those targets exit early before we reach the opt target.

If there's no plan for this code to be used in production, I'm not sure it's worth adding this extra logic which needs to be maintained. If it's just for being played around with in testing then the extra time it takes to build isn't so bad? Unless I'm misunderstanding the problem?

[wenju-he]

just curious would this Pr unifies the this code and avoid the early exit and duplicate code for prepare target?

[wenju-he]

let me know if it worth to update PR to unifies the code path and avoids early exit. Otherwise I'll close this PR.

The original request of this PR is to disable opt -O3. But things has changed and the request is irrelevant.
Though in practice it might still make sense to apply the PR to avoid code diverge like for SPIR-V.

[frasercrmck]

Yes, that might be good actually. Then if SPIR-V did ever want to apply optimizations, it could do so, and it ensures this path is tested.

Note though we'd still need the special-case after the opt target, because SPIR-V will continue to want to run llvm-spirv and exit. It also doesn't run the ${prepare_builtins_exe} utility so needs to exit before that point. I don't know enough about prepare-builtins to say whether SPIR-V could also make use of it: it seems to strip opencl.ocl.version metadata and set every external definition to linkonce_odr linkage. I'd need to investigate what's going on there.

[wenju-he]

close this PR. Original intention of disabling opt -O3 on downstream project was changed.