Page MenuHomePhabricator

Let TableGen write output only if it changed, instead of doing so in cmake, attempt 2

Authored by thakis on Dec 18 2018, 11:05 AM.



Removes one subprocess and one temp file from the build for each tablegen invocation.

This attempts to reland r330742. r330742 got reverted because some people reported that llvm-tblgen ran on every build after it.

This could happen if the depfile output got deleted without deleting the main .inc output. To fix, make TableGen always write the depfile, but keep writing the main .inc output only if it has changed. This matches what we did in cmake before.

dberris, could you check if things are better for you with this version?

Diff Detail


Event Timeline

thakis created this revision.Dec 18 2018, 11:05 AM

Apologies for the delay, but I'm giving this a go now.

dberris accepted this revision.Dec 19 2018, 1:41 AM

LGTM -- cannot reproduce the original issue with this patch.

This revision is now accepted and ready to land.Dec 19 2018, 1:41 AM
This revision was automatically updated to reflect the committed changes.

Thanks for checking! Relanded in r349624.

I'm not sure this is a significant problem at the moment (I'll find out the next time I'm debugging :-)), but just to mention it. One unfortunate consequence of this is presumably that no output is written to a file when tblgen crashes because the output was only written to memory and that was lost in the crash. FWIW, being able to see the successfully-produced output has been fairly handy for me in the past as it helps to establish the context which I can then use to minimize the input or determine a strategy for debugging it. This can be particularly helpful when lldb fails to print important variables (which seems to happen often for me), and the same stack trace occurs frequently during execution. We might want to consider an option that makes it write directly to a file if that does become a problem.

@thakis As discussed on PR43385 I think we're going to revert this as it doesn't seem to be working correctly on MSVC in conjunction with rL371683

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 4:43 AM