The idea of introducing SizeOfInt to TargetLibraryInfo looks quite reasonable to me, although I am not too familiar with this part of target description.

In D86547#2284095, @MaskRay wrote:

The (aWidth - 1) - clzsi(a) change is correct, but why is the ctz change?

See rG553833958fd as an example.

Ping.

Ping.

Thank you very much for review!

Update after the latest comments.

Re-upload after amending parent diff + add minor clarification.

Clarify rounding-related part of function.

In D86453#2241702, @zatrazz wrote:

Hi atrosinenko, do you think this patch need any more change on the testing side?
The fp_lib.h/int_lib.h change would most likely require in a more complex without
much gain in organization imho.

Re-upload after amending parent diff.

Add more clarifications, fix explanation for "why it is enough to adjust only once in case of overflow".

Re-upload after changing parent diff.

Add some other explanations.

This update is expected to be completely NFC w.r.t. code behavior and significantly clarify the proof up to the end of half-width iterations.

No-change re-upload: rebase onto current master branch.

No-change re-upload: rebase onto current master branch.

Pinging @echristo in case there is some trivial way to allocate a calling convention ID (but probably there is not, unless GCC allocated one already). As I understand, without this ID the debugger may show some misleading output for a limited number of LibCalls. On the other hand, it will not help either, unless the debugger knows this ID.

• Rebase onto current master branch
• Add a dummy switch case with FIXME, as suggested by @aaron.ballman
• Applied a couple of style fixes proposed by linter

Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.

Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.

Replaced this by D86546: [compiler-rt][builtins] Use explicitly-sized integer types for LibCalls and D86547: [compiler-rt][builtins] Use c[tl]zsi macro instead of __builtin_c[tl]z. These two patches include everything except for factoring out src_rep_t_clz() and rep_clz() to clzdi() for now.

Uploaded, thank you!

Explicilty mention that UBSan have to be manually configured in trap-on-error mode to operate of builtins library.

Sorry for such an iterative feedback. A couple of general thoughts:

• It may be worth looking at D86453: [AArch64] Support conversion between fp16 and fp128: if I get it right, it introduces a TYPE_FP16 macro to be used instead of uint16_t when using it instead of half-size float. When one of this patch and D86453 lands, the other one will probably need some updates
• Unlike many other tests, this one always checks all the test cases instead of failing with non-zero exit code on the first failed one. If something goes wrong with this libcall, build log can be flooded with up to 65k warning lines. Still I'm not sure whether this is an issue.

As far as testing, both the brute force and the two-way algorithms pass the unit tests and the fuzz test for strstr.

Re-upload with properly set parent review.

Reupload.

Re-upload after parent commits were changed.

Addressed the review comments mostly by clarifying the explanations.

Explicitly mark .inc file as a C source like is already done for almost all *_impl.inc files.

Explicitly mark .inc file as a C source like is already done for almost all *_impl.inc files.

Thanks @ldrumm! Some comments on the test follow.

Thank you @sepavloff !

On one hand, this clzdi() implementation should be completely harmless when fallback is implemented via clzsi(). On the other hand, it may possibly be completely removed. This code snippet mimics one already existing for clzsi in int_types.h and for currently supported targets it can probably be even as simple as this (provided we can use C99):

Fix the misleading comment.

Rebase the entire patch stack against the up-to-date master and re-upload.

Rebase the entire patch stack against the up-to-date master and re-upload.

Rebase the entire patch stack against the up-to-date master and re-upload.

Rebase the entire patch stack against the up-to-date master and re-upload.

Thank you for the test cases. Looks like it is worth completely rewriting the three tests as table-driven tests, so for example adding four cases for [+-]Inf / [+-]0.0 would be almost as easy as testing only one of them (something similar is already implemented for udivmod?i4_test.c). The differences between adjacent test cases would be more visually obvious, as well.

Address the review comments.

Maybe I overestimated similarity of byval and recently introduced byref... Looks like some aliasing restrictions are not mentioned in LLVM Language Reference. For example, the only way for Clang to emit the byref attribute to LLVM IR I know about is via ABIArgInfo with Kind == IndirectAliased introduced in D79744: clang: Use byref for aggregate kernel arguments - and it has a note that "The object is known to not be through any other references for the duration of the call, and the callee must not itself modify the object". This seems to be necessary for correctness of this transformation. If LLVM Language Reference does not mention such restrictions intentionally, then it may be clang that should be responsible for such optimizations.

Add a test for insufficiently aligned source.

Update with implicit pointers being passed through.

Restrict transformation to passing through byref arguments, not arbitrary pointers.

In D84602#2206512, @aaron.ballman wrote:

I still think this case needs some work to keep the bots happy. I would probably go with:

case CC_MSP430Builtin:

// FIXME: Currently unsupported
break;

But perhaps @echristo has opinions since this involves debugging information.

Hmm, this probably needs further investigations... On one hand, condition codes such as ISD::SETNE are explicit "Don't care operations: undefined if the input is a nan" - that is, exactly how the __mspabi_cmp[fd] are described in MSP430 EABI document. On the other hand, how a fully fledged comparison libcall can be specified then?

A shy ping :)

Restore test formatting.

Ping.

Ping.

Thanks @aykevl! This could be useful for MSP430 as well. Especially, the tests would be worth reusing even for target-specific assembly re-implementation of these routines.

A bit of comment fine-tuning. :)

On linter diagnostics: error messages are due to linter trying to lint *.inc file that is not self-contained by design. D85731: [NFC][builtins] Make softfloat-related errors less noisy tries to make those errors more meaningful, at least.

Ping

Re-upload after D85731: [NFC][builtins] Make softfloat-related errors less noisy to get rid of "error: unknown type name 'fp_t'" and similar clang-tidy diagnostics for fp_div_impl.inc.

Re-upload after D85731: [NFC][builtins] Make softfloat-related errors less noisy to get rid of "error: unknown type name 'fp_t'" and similar clang-tidy diagnostics for fp_div_impl.inc.