Added a cost-benefit option: --combiner-select-seq-min-cost-benefit
Added a check: <ConstantSDNode>.getAPIntValue().getMinSignedBits() <= 64

For the test case from https://bugs.llvm.org/show_bug.cgi?id=51147 the (cross-compiling on RHEL8/x86_64) build times:
without -mllvm --combiner-select-seq: real 24m16.804s
with -mllvm --combiner-select-seq: real 0m0.897s
with about 25% code size reduction under the option.

Thanks to the Works On Arm program https://www.arm.com/solutions/infrastructure/works-on-arm for providing an equinix-metal-arm64-cluster by Equinix https://www.equinix.com for developing this solution.

In D122517#3409266, @MaskRay wrote:

Bazel uses -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted".
For reproducibility purposes, this is sufficient. I have two questions:

• Do we need the arbitrary time feature?
• Should an arbitrary string like "redacted" be supported?

In D122517#3409248, @appcs wrote:

(From D121040) Please let me know about the following and I will incorporate your feedback and submit subsequent patches for addressing these questions:

a. Disallow the simultaneous use of -Wdate-time/-Wpch-date-time and -ffixed-date-time=<timestamp>?
b. Disallow -ffixed-date-time=<timestamp>:

0. if PCH's are used
1. in PCH compilation

Also, (just as -Wdate-time warns on TIMESTAMP use), do we (in a separate patch), extend the scope of -Wpch-date-time to cover the use of TIMESTAMP is in a PCH?

Thank you.

In D122517#3412464, @appcs wrote:

In D122517#3409266, @MaskRay wrote:

Bazel uses -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted".
For reproducibility purposes, this is sufficient. I have two questions:

• Do we need the arbitrary time feature?

If you are saying that defining DATE, TIMESTAMP, TIME to an arbitrary time value, then that is the definition of the problem that we are trying to solve; else, please clarify. Thanks.

• Should an arbitrary string like "redacted" be supported?

Via -ffixed-date-time the value would have to be formatted e.g. 1969-12-31T23:59:59 (RFC3339 without fractional seconds and timezone); else, please clarify. Thanks.

• Re-spun with std::get_time().
• Expanded regression test with multiple error scenarios.

In D122517#3409266, @MaskRay wrote:

Bazel uses -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted".
For reproducibility purposes, this is sufficient. I have two questions:

• Do we need the arbitrary time feature?

(From D121040) Please let me know about the following and I will incorporate your feedback and submit subsequent patches for addressing these questions:

I will port https://reviews.llvm.org/D23934 to top of trunk, thank you.

In D23934#3314277, @appcs wrote:

Hello, Will this feature be committed? Thanks!

In D121040#3365149, @bruno wrote:

...
Also, along the lines of similar questions in D23934, how does this affect -Wdate-time and -Wpch-date-time?

In D121040#3362536, @thakis wrote:

Why isn't D23934 sufficient? What do these flags with very long names do that ffixed-date-time doesn't?

In D23934#3355355, @emaste wrote:

The original author appears to have given up on it. Someone will need to rebase and work on pushing it forward

Hello, Will this feature be committed? Thanks!

Thank you @Szelethus for the guidance. Thank you @NoQ for further input. Please see inline:

ping

In D53185#1284536, @Szelethus wrote:

Let's talk about more general topics: Your checker's name is "TimeChecker", not "Y2K38Checker" or something similar, do you have plans on expanding this checker to cover more time.h related functionality?

Incorporated code review feedback; thank you @Szelethus and @george.karpenkov

Re-spun with feedback received into: D53185

Thank you very much for the feedback. I have added a little functionality to demonstrate the working of a prototype checker for a some sample Y2K38 issues. There are some TODO's in the test files indicative of some subsequent checking that we will add.

Thank you, everybody, for the valuable inputs.

In D28075#645429, @aprantl wrote:

There are several options I could think of:
Either surfacing it through a driver option, enabling it at -Og, or benchmarking it and arguing to enable it by default at some optimization settings.

• Added documentation to source code on -mergefunc-preserve-debug-info behaviour and noted the difference from the base -mergefunc
• Elaborated & consolidated comments in regards MergeFunctionsPDI for MergeFunctions::writeThunk()
• Made suggested stylistic changes.

In D28075#644203, @aprantl wrote:

Added some more stylistic changes.

Is this good?

Submitting ticked off items.

Under -mergefunc-preserve-debug-info

• A thunk's call site is preserved to point to the thunk when both occur within the same translation unit.

In D28075#638292, @aprantl wrote:

In D28075#638014, @appcs wrote:

What should -mergefunc-preserve-debug-info do about the thunk's call to the shared implementation?
Mark as tail call or not? The existing -mergefunc behaviour is to mark it as a tail call. We could leave it as is,
unless someone specifically asks for a change under -mergefunc-preserve-debug-info; would that be OK?

I think marking it as a tail call makes most sense.

In D28075#637947, @aprantl wrote:

In D28075#637667, @appcs wrote:

In D28075#637188, @aprantl wrote:

Call sites of the merged function occurring from within the TU of the merged function's definition will point directly to the shared implementation and call sites of the merged function that are external to the TU of the merged function's definition call the thunk for the merged function (which tail call's the shared implementation passing forward the incoming arguments). This is existing behaviour under -mergefunc and remains unchanged under -mergefunc-preserve-debug-info (except for the tail call part).

Is this the right thing to do, though? I would think that for better debugability users would always prefer the version with the thunk even in the current TU. Then again, subsequent optimization passes would likely inline the thunk and thus potentially undo most of the effect. Have you experimented with this variant? Would the debug info from the thunk survive?

Thank you; I agree that this greatly enhances the debug experience. Automatically modifying the call site with a direct call to the shared implementation instead of (what is now) the thunk, when within the TU, is confusing when debugging. Stepping into the thunk from the call site and from within the thunk, into the shared implementation keeps the debug experience uniform for internal and external call sites of functions that become thunks. (If you ask me, this is similar in spirit to not marking the thunk's call to the shared implementation as a tail call to help debugging - the full back trace is the same in both cases and otherwise, there is a certain element of surprise in both cases. Basically, we are saying that under -mergefunc-preserve-debug-info we trade off optimization partly (at -O0) to aid debugability, modifying the transformation slightly, to make the execution flow explicit).

Yes. I think this is the right approach. When using -mergefunc-preserve-debug-info we should always call the thunk even in the same TU.

In D28075#637188, @aprantl wrote:

Call sites of the merged function occurring from within the TU of the merged function's definition will point directly to the shared implementation and call sites of the merged function that are external to the TU of the merged function's definition call the thunk for the merged function (which tail call's the shared implementation passing forward the incoming arguments). This is existing behaviour under -mergefunc and remains unchanged under -mergefunc-preserve-debug-info (except for the tail call part).

Is this the right thing to do, though? I would think that for better debugability users would always prefer the version with the thunk even in the current TU. Then again, subsequent optimization passes would likely inline the thunk and thus potentially undo most of the effect. Have you experimented with this variant? Would the debug info from the thunk survive?

MergeSimilarFunctions demonstrates great improvement on Cisco software, so this is of great value to us. We have some improvements upon the base work to improve the compile time.