[CMake][Windows] Fix Debug config build when using MSBuild #111765

wjristow · 2024-10-09T21:48:13Z

Commit 95d4506 caused a failure in a debug-build with MSBuild. That commit conditionally makes an assignment to a cmake variable, but suppresses the assignment when LLVM_TABLEGEN is precicely "llvm-tblgen". This commit updates that change, so that the assignment is suppressed when LLVM_TABLEGEN logically is "llvm-tblgen" (i.e., it also suppresses it for "llvm-tblgen.exe", or for a full pathname that ends with llvm-tblgen" or "llvm-tblgen.exe").

chapuni

I don't understand your real issue. Could you elaborate?

If you specifies LLVM_TABLEGEN with no reasons, I suggest you stopping it.
Do you use LLVM_OPTIMIZED_TABLEGEN?

chapuni · 2024-10-14T10:16:12Z

llvm/cmake/modules/TableGen.cmake

  if("${target}" STREQUAL "llvm-min-tblgen"
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL ""
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL "llvm-tblgen")
-    set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")


I suppose ${LLVM_TABLEGEN} is llvm-tblgen by default.
You can modify it if you want to use out-of-tree llvm-tblgen.

We don't use an out-of-tree llvm-tblgen. For me, on input to this block, ${LLVM_TABLEGEN} is of the form:
C:\path\to\dir-within-build-dir\bin\llvm-tblgen.exe

Again, ${LLVM_TABLEGEN} is llvm-tblgen by default.

chapuni · 2024-10-14T10:20:02Z

llvm/cmake/modules/TableGen.cmake

-    set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")
+      AND NOT "${LLVM_TABLEGEN}" STREQUAL "")
+    # Extract base filename from full path.
+    get_filename_component(RAW_LLVM_TABLEGEN ${LLVM_TABLEGEN} NAME_WE)


I wonder get_filename_component would be applicable here.
Would it match if ${LLVM_TABLEGEN} points the explicit out-of-tree /path/to/llvm-tblgen.exe?

${LLVM_HOST_EXECUTABLE_SUFFIX} may be available here, I guess.

I'm not sure I understand the question. For me, when ${LLVM_TABLEGEN} is of the form:
C:\path\to\dir-within-build-dir\bin\llvm-tblgen.exe

the line:
get_filename_component(RAW_LLVM_TABLEGEN ${LLVM_TABLEGEN} NAME_WE)

sets RAW_LLVM_TABLEGEN to simply llvm-tblgen, which is what I want -- essentially, enabling the portion of the change that suppresses the setting of ${project}_TABLEGEN_DEFAULT that you made in the commit 95d4506.

wjristow · 2024-10-16T02:42:39Z

Thanks for the comments @chapuni.

I don't understand your real issue. Could you elaborate?

Sorry. I should have explained more.

I also want to let you know that I don't have much experience with CMake, so I may be approaching this the wrong way (and so I can abandon this, if that is your opinion after I explain more).

I'll elaborate here, and a bit more in responses to the questions where you left comments in the code. But the big picture is that in the block that was added in 95d4506:

  # FIXME: Quick fix to reflect LLVM_TABLEGEN to llvm-min-tblgen
  if("${target}" STREQUAL "llvm-min-tblgen"
      AND NOT "${LLVM_TABLEGEN}" STREQUAL ""
      AND NOT "${LLVM_TABLEGEN}" STREQUAL "llvm-tblgen")
    set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")
  endif()

for my running on Windows, on input in my run, ${LLVM_TABLEGEN} is of the form:
C:\path\to\dir-within-build-dir\bin\llvm-tblgen.exe

and ${target} is llvm-min-tblgen (${project} is LLVM_HEADERS in this instance, but other values later).

So the set statement:
set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")

is enabled, setting ${LLVM_HEADERS_TABLEGEN_DEFAULT} to C:\path\to\bin-dir-in-build-dir\bin\llvm-tblgen.exe. And that later causes the build to fail. (On input, ${LLVM_HEADERS_TABLEGEN_DEFAULT} was llvm-min-tblgen. And if it had been left unchanged as that value, my build passes.)

Your commit 95d4506 appeared to me to be intending to do:
For the target "llvm-min-tblgen", update ${project}_TABLEGEN_DEFAULT to the value in ${LLVM_TABLEGEN}, unless ${LLVM_TABLEGEN} is the string "llvm-tblgen" (in which case leave ${project}_TABLEGEN_DEFAULT unchanged).

I don't understand the reason for updating ${project}_TABLEGEN_DEFAULT, but it seemed to me that if the update was to be suppressed when ${LLVM_TABLEGEN} is the string "llvm-tblgen", then it probably should also be suppressed if it's a full path to "llvm-tblgen" (and if it has the executable-extension appended: ".exe" for Windows). That's what my PR is intending to do.

Do you use LLVM_OPTIMIZED_TABLEGEN?

Yes, we do use LLVM_OPTIMIZED_TABLEGEN

wjristow · 2024-10-17T07:36:03Z

One additional thought. After thinking about the explanation I wrote up yesterday, I feel like there's a cleaner way to implement the fix. Here is a diff, showing the change that I think is cleaner:

diff --git llvm/cmake/modules/TableGen.cmake llvm/cmake/modules/TableGen.cmake
index ffcc718b4777..f782cf112fac 100644
--- llvm/cmake/modules/TableGen.cmake
+++ llvm/cmake/modules/TableGen.cmake
@@ -190,9 +190,13 @@ macro(add_tablegen target project)
   endif()

   # FIXME: Quick fix to reflect LLVM_TABLEGEN to llvm-min-tblgen
+  set(RAW_LLVM_TABLEGEN ${LLVM_TABLEGEN})
+  if(NOT "${RAW_LLVM_TABLEGEN}" STREQUAL "")
+    get_filename_component(RAW_LLVM_TABLEGEN ${RAW_LLVM_TABLEGEN} NAME_WE)
+  endif()
   if("${target}" STREQUAL "llvm-min-tblgen"
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL ""
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL "llvm-tblgen")
+      AND NOT "${RAW_LLVM_TABLEGEN}" STREQUAL ""
+      AND NOT "${RAW_LLVM_TABLEGEN}" STREQUAL "llvm-tblgen")
     set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")
   endif()

I haven't updated the commit in this PR, but I'll do that tomorrow, for clarity. This updated approach, sets RAW_LLVM_TABLEGEN to be a "cleaned up" version of LLVM_TABLEGEN (removing the extension and the the full path, if any), and then uses that cleaned up version instead of LLVM_TABLEGEN directly when deciding whether to suppress the assignment to $(project)_TABLEGEN_DEFAULT that was done in 95d4506.

wjristow · 2024-10-18T17:27:52Z

I haven't updated the commit in this PR, but I'll do that tomorrow, for clarity.

Done.

wjristow · 2024-10-29T06:36:08Z

Hi @chapuni, Does my explanation above, and updated approach make sense to you? From what I understand, my proposed change is generalizing the change that you made in 95d4506, so it seems sensible to me.

But as I said earlier:

I also want to let you know that I don't have much experience with CMake, so I may be approaching this the wrong way (and so I can abandon this, if that is your opinion after I explain more).

So if you see a better solution, or a way for me to avoid the problem, please let me know.

chapuni · 2024-10-29T14:53:06Z

I have to reconfirm it. Let me prepare the msbuild environment. It'll take time.

wjristow · 2024-10-29T18:45:02Z

I have to reconfirm it. Let me prepare the msbuild environment. It'll take time.

I appreciate you looking into it @chapuni. No problem about the timing!

chapuni

I've reconfirmed LLVM_TABLEGEN is llvm-tblgen by default.

VS Buildtools
CMake-3.30.5
Regardless of LLVM_OPTIMIZED_TABLEGEN

Don't override LLVM_TABLEGEN without the reason. Use LLVM_NATIVE_TOOLS_DIR for the purpose.

chapuni · 2024-10-30T03:41:15Z

llvm/cmake/modules/TableGen.cmake

  if("${target}" STREQUAL "llvm-min-tblgen"
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL ""
-      AND NOT "${LLVM_TABLEGEN}" STREQUAL "llvm-tblgen")
-    set(${project}_TABLEGEN_DEFAULT "${LLVM_TABLEGEN}")


Again, ${LLVM_TABLEGEN} is llvm-tblgen by default.

chapuni · 2024-10-30T03:43:30Z

llvm/cmake/modules/TableGen.cmake

  # FIXME: Quick fix to reflect LLVM_TABLEGEN to llvm-min-tblgen
+  set(RAW_LLVM_TABLEGEN ${LLVM_TABLEGEN})
+  if(NOT "${RAW_LLVM_TABLEGEN}" STREQUAL "")
+    get_filename_component(RAW_LLVM_TABLEGEN ${RAW_LLVM_TABLEGEN} NAME_WE)


Would it match if ${LLVM_TABLEGEN} points to X:/out-of-tree/path/to/llvm-tblgen.exe? I don't think it'd be desirable.

wjristow · 2024-10-31T06:57:22Z

I've reconfirmed LLVM_TABLEGEN is llvm-tblgen by default.
* VS Buildtools

* CMake-3.30.5

* Regardless of `LLVM_OPTIMIZED_TABLEGEN`
Don't override LLVM_TABLEGEN without the reason. Use LLVM_NATIVE_TOOLS_DIR for the purpose.

Thanks for that feedback, and the advice about LLVM_NATIVE_TOOLS_DIR @chapuni . That looks very helpful. Our build-infrastructure doesn't invoke cmake directly, but instead does it through some other code. I'll look into that, but I'm optimistic that your LLVM_NATIVE_TOOLS_DIR point will be very helpful.

wjristow · 2025-01-02T19:00:18Z

Thanks for your help and your patience on this, @chapuni.

Don't override LLVM_TABLEGEN without the reason. Use LLVM_NATIVE_TOOLS_DIR for the purpose.

That looks very helpful. Our build-infrastructure doesn't invoke cmake directly, but instead does it through some other code. I'll look into that, but I'm optimistic that your LLVM_NATIVE_TOOLS_DIR point will be very helpful.

I've looked into this further, and I see a way to modify our build-infrastructure so that it doesn't override LLVM_TABLEGEN. So I'm abandoning this pull-request. Thanks again.

wjristow requested a review from chapuni October 9, 2024 21:48

llvmbot added the cmake Build system in general and CMake in particular label Oct 9, 2024

chapuni reviewed Oct 14, 2024

View reviewed changes

[CMake][Windows] Cleaner implementation of LLVM_TABLEGEN check

00be4e6

chapuni requested changes Oct 30, 2024

View reviewed changes

wjristow closed this Jan 2, 2025

wjristow deleted the cmake-tablegen-win branch January 2, 2025 19:01

[CMake][Windows] Fix Debug config build when using MSBuild #111765

[CMake][Windows] Fix Debug config build when using MSBuild #111765

Uh oh!

Conversation

wjristow commented Oct 9, 2024

Uh oh!

chapuni left a comment

Choose a reason for hiding this comment

Uh oh!

chapuni Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

wjristow Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

chapuni Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

chapuni Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

wjristow Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjristow commented Oct 16, 2024

Uh oh!

wjristow commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjristow commented Oct 18, 2024

Uh oh!

wjristow commented Oct 29, 2024

Uh oh!

chapuni commented Oct 29, 2024

Uh oh!

wjristow commented Oct 29, 2024

Uh oh!

chapuni left a comment

Choose a reason for hiding this comment

Uh oh!

chapuni Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

chapuni Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

wjristow commented Oct 31, 2024

Uh oh!

wjristow commented Jan 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wjristow Oct 16, 2024 •

edited

Loading

wjristow commented Oct 17, 2024 •

edited

Loading