<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/125779>125779</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            Can recommendation for out-of-tree projects to `add_definitions(${LLVM_DEFINITIONS})`  be retired?
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          christopherbate
      </td>
    </tr>
</table>

<pre>
    TLDR: Recently I spent several hours debugging compilation errors caused by differences in how a bunch of LLVM and MLIR-related projects, both in the OSS world and local to my employer, utilize the `LLVM_DEFINITIONS` CMake variable when finding and setting LLVM or MLIR in an out-of-tree CMake project. Rather than fixing them all independently to be uniform, I'm wondering why downstream projects need to utilize `LLVM_DEFINITIONS` at all (see below for the LLVM CMake doc guidance on this variable).

The `LLVM_DEFINITIONS` variable is defined like this in `HandleLLVMOptions.cmake`:

```
function(get_compile_definitions)
 get_directory_property(top_dir_definitions DIRECTORY ${CMAKE_SOURCE_DIR} COMPILE_DEFINITIONS)
  foreach(definition ${top_dir_definitions})
 if(DEFINED result)
      string(APPEND result " -D${definition}")
 else()
      set(result "-D${definition}")
    endif()
  endforeach()
 set(LLVM_DEFINITIONS "${result}" PARENT_SCOPE)
endfunction()
get_compile_definitions()
```

So apparently, it just gets all the directory COMPILE_DEFINITIONS in the top-level source directory and joins them into a space-separated list as a string, adding the `-D` prefix.

Searching uses of `LLVM_DEFINITIONS` in both in-tree and  out-of-tree projects like StableHLO [see use here](https://github.com/openxla/stablehlo/blob/main/CMakeLists.txt#L176) , shows that many callers are invoking `add_definitions(${LLVM_DEFINITIONS})`, which is wrong because `add_definitions("-Dname1=1 -Dname2=2")` will result in `-Dname1='1 -Dname2=2'` being added to the compiler flags.

Because of this, the official [LLVM CMake documentation section "Embedding LLVM in your project"](https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project), was changed [years ago](https://reviews.llvm.org/D103044) to recommend this:

```
  include_directories(${LLVM_INCLUDE_DIRS})
 separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
 add_definitions(${LLVM_DEFINITIONS_LIST})
```

Rather than change all the callers across multiple projects, I'm wondering why downstream projects which embed LLVM need to make an `add_definitions` call at all. 

The definitions in `LLVM_DEFINITIONS` are themselves derived from the `CMAKE_SOURCE_DIR` and thus wouldn't they already apply wherever `HandleLLVMOptions` is being invoked in the build? Can we just remove this recommendation/requirement? 

In my setup, I typically embed LLVM by building-from-source in a super project using `add_subdirectory` to build LLVM. Invoking the recommended sequence above afterwards just causes a duplication of flags. Worse, if you have multiple other embedded out-of-tree MLIR/LLVM projects, and they also invoke the above sequence, then your flags will duplicate again. I actually observed on a very old system that this could cause some sort of overflow in the commands executed by cmake. 




</pre>
<img width="1" height="1" alt="" src="http://email.email.llvm.org/o/eJycV1uP4rgS_jXulxIoGJrLAw8MFy06dPeo6d2j84ScuEI849hZ24Hh_PqjchLonqH3rHaEphFxfa7LV19VhPfqaBDn7PELe1w9iDoU1s2zwikfbFWgS0XAh9TKy_xtt3plwwW8YoYm6AtswVdoAng8oRMaCls7DxLT-nhU5giZLSulRVDWADpnnYdM1B4lpBeQKs_RocnQgzJQ2DMISGuTFWBz2O3-eAJhJDzttq89h1oElFA5-w2z4BlfQmpDQYahQHjZ7-FsnZbRRNtMaAgWygtgWWl7QUcWdVBa_RejBRsndMVhtd5sn7dv25fnPRsnsHwS3xFOwimRaoRzgQZyZSRFQ9AeQ6Dv0T3ronfkhDBg69CzeS84xBam9bYPryIU6CAUgtB-EEAosAShNSgjsUIjm4wGCylCbVRuXUk-bxmflHC2RqIju3NxAWnPxgeHorwmBAyiJOsuxk_iEyFeyvjUI0KK2p4hty6mJMbUeC5tBsdaSWEyBEs5Vv6aFcZnfZYsWLJ4-zyR1xQqIkSuDErQ6js2UMqQ3W_CSI1k_VIRR3w_K8V3ZOOEDRfNDfS9-SSLvDYZHWN8esRwaMiFh4iuoj3jM5YsgJ5K5TAL1l0OlbMVunBhfBpsRQ_em8Bq-7pevr28_gcYH7HJl-XT4l_rw_7l99fl-rDavrLJCpYvT1-3u_WHGJurKHkosoLx6Q20RbpzG5usWkOVMz6NeOsVOPS1Dh0k_fOBqs34dPH16_q5OwGMc-itGvgbbETlrTlqj4xPP4BhYHx6g_g_CACARkYHu1_QyFug7Y8N6s-1h4hD6G1QERm-Ll7Xz2-H_fLl67qxJ8hbQZvfPi1r-_w9G1iy2FsQVSVcbB1qFhXgW-0DEcBHnhOtr0y4V8ZOQIKtehpPqMHb2mXvrajtv1llfNOzygQLAnwlMux5rISLyqSVDyA8PWlLtwQhZdvqxPbeivqicpirH23_7FG4rKAztUdPsvdJNynTyV2jL-TTB8G56kDssX2g1vtt9wLs8Qs1eu0RCnTIHleMT4sQKk8txjeMb44qFHXaz2zJ-MZWaH5owfjGR4xCW8Y3qbYp45tSKMP4JkrETvng--FHYHy4G0zGjM-AYvaFPVOmRIBSmAtkQmt0HoRDUOZkv1O0bJwIKX8pMbHml-ibjhknBH4uVFaQopydNUdIMU6Tz-B4b2VEiQM2XA2g-c7ZcMVbpo8TOCutu9ZqJOlmwvjkJ6sJmaQYR4GUjdxSbVvGOsi1OPq2tF9a32weFY-8p7M2z1WmhKbCfNTbukQTmlHpMWtlhK_LFBsWxdPKwMXWris3RXKnolqfyr51R8Y30ma-K1m_CKVmfIgdZo8O9pTpEWbvijmLqRYeskKYI0ry9YKCini09-5zeFJ49v13964GyTAZjYgVwYLDzJYlGtnk4p64AyiT6VriVbkV_sSK7fNy9_sqivL-JqRdCx6EO8Yc-juqdNht92_wvHjb_rE-LF-enhbPK_hLwiUL-LsUjeBXs5806v3ob_J5FaZra2TOeg9lrYOqNH5Ycv7u-G86I1a2YUq3D9BApe3kTo-Mk-hCuxP04TbT38_Hpi_u7hIurlKlR31CmvJOnVBC7mzZad4vw5TMIg1qD2dba2kYnwQ6fgGhHQp5IU3XF1q9HK2VdxeFqIm-bcYoKyg7KU9rpSUbbmApDJyxmQgOS3tql48rG0UzfDYO_6yVQyIP2TV52BpaHz2GuoqFgHCpFOXr8j7N6aW5j7qJAu-144NWQvB1hddehdq_kz5fp9cJQ8HQ3kc4EbUP204pKaCrv0j75581rcwgUopH5AHdWTjpmzCj5tAQknWlVdbIic1bZYJ_W0fLwRJUTjoChTjhjXc2MrVRB5QfxgutuYxvYsjv6dnUMtbO27YQ0efGvc7bVv1a8YrONNrbuYkgjkKZPmxBZKGOWbapR0eEspTLE7oLWC3BX3zAspkwsZwZ0aiJHLwt6T8XKGh7QpfTgtsSg7IojPSAPzCrQ_MWEjfOjvvN50HOh3I2nIkHnA8mw-lkkAwHs4ding8yMc4znE7ouZzhLBtmQz4YZDyfDUePD2rOE_6Y8GTEh8njYNYfiNFoPJomOOGYpnnCRgmWQumrVD4o72ucD_jjZDJ70CJF7btXMTeP-pzWR89GiY4D92oXVNA4J4p_pHPc5u9uBsH-o7kL9ELiMCiH1FUPtdPzv1ggyMH2z22ibGKUNIjaQE9z_r8AAAD__yOEwgo">