The change looks correct. Let's wait for somebody else to give their final approval.

In D139170#3965850, @FreddyYe wrote:

In D139170#3965814, @mgehre-amd wrote:

Do other targets not support > 128 bit integers, or is this PR only the first conservative step of lifting the limit?

I temporary only enabled > 128 bit FP conversion for X86 in https://reviews.llvm.org/D137241, since I highly relied on end-to-end tests to implement that pass and I don't have other targets' environment to verify. So same to this patch, I only lift the limit for X86. Hope other targets will follow us to enable for themselves. WDYT?

Do other targets not support > 128 bit integers, or is this PR only the first conservative step of lifting the limit?

I think this was the last reason for restricting _BitInt to <= 128 by default in clang. Are you planning to create a PR to lift that restriction now?

I'm scared about missing some subtle issue in the implementation, but I think the test cases on the other PR should cover that.

Rebase main to update alloca-bitcast.ll

Split tests and remove explicit metadata numbers

Great that you worked on this!
I will have a deeper look at the end of this week.

Hey @jmorse, I talked to @nikolaos-amd offline and will take over this revision.

In D133691#3785268, @chapuni wrote:

Seems tests are failing when x86 is not configured.
See clang-aarch64-quick.

Use a field instead of virtual functions

In D130076#3777630, @aaron.ballman wrote:

In D130076#3771515, @FreddyYe wrote:

LGTM. Thanks @mgehre-amd for your great work to implementing this feature!

Agreed, thank you for this work! Do you agree that we can now bump BITINT_MAXWIDTH in Clang above 128 now, or are we still missing support for that (floating-point conversions were also a problem IIRC)?

Thanks back to everyone who helped by reviewing!
Unfortunately, float-to-big-int and and big-int-to-float conversions still crash in the backend.
I want to take a look at this next.

In D130076#3772580, @vitalybuka wrote:

Breaks https://lab.llvm.org/buildbot/#/builders/85/builds/10520

******************** TEST 'LLVM :: Transforms/ExpandLargeDivRem/udiv129.ll' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/opt -S -expand-large-div-rem < /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/Transforms/ExpandLargeDivRem/udiv129.ll | /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/Transforms/ExpandLargeDivRem/udiv129.ll
--
Exit Code: 1

Command Output (stderr):
--
/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/Transforms/ExpandLargeDivRem/udiv129.ll:6:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: _udiv-special-cases:
              ^
<stdin>:5:19: note: scanning from here
define void @test(i129* %ptr, i129* %out) #0 {
                  ^
<stdin>:7:9: note: possible intended match here
 %res = udiv i129 %a, 3
        ^

Input file: <stdin>
Check file: /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/Transforms/ExpandLargeDivRem/udiv129.ll

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: ; ModuleID = '<stdin>' 
          2: source_filename = "<stdin>" 
          3:  
          4: ; Function Attrs: nounwind 
          5: define void @test(i129* %ptr, i129* %out) #0 { 
next:6'0                       X~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          6:  %a = load i129, i129* %ptr, align 4 
next:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          7:  %res = udiv i129 %a, 3 
next:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~
next:6'1             ?                possible intended match
          8:  store i129 %res, i129* %out, align 4 
next:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          9:  ret void 
next:6'0     ~~~~~~~~~~
         10: } 
next:6'0     ~~
         11:  
next:6'0     ~
         12: attributes #0 = { nounwind } 
next:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>

• Simplified tests to only check for NOT: call

In D130076#3751142, @pengfei wrote:

The tests are too prolix to check the correctness and it's fragile to use so many registers. I suggest we remove those tests and just do run-time time in llvm-test-suite.

Yes, you are right. I will restrict the IR tests to maybe just check for the absence of a call instructions? And at least to check that llc doesn't assert like it used to do.

Hey, thanks for the PR!
In the test cases, I think that both of the modified transformations (simplifyAllocaArraySize & PromoteCastOfAllocation) trigger (one after another).

Ping :)

In D130079#3670678, @LuoYuanke wrote:

I recommend to revert this patch after the new patch landed in llvm, so that it doesn't break existing functionality.

Rename expandlargedivrem to expand-large-div-rem
Fix filenames in comments

In D130076#3728779, @pengfei wrote:

Is this to address @arsenm's comments at https://reviews.llvm.org/D126644#3581469?

Thanks @aaron.ballman for helping out with the reviewers!

Rebased

Friendly ping :-)
I think I addressed all comments. Is somebody able to approve this?

In D126644#3686479, @hiraditya wrote:

We usually provide a runtime library for specialized functionalities like this. Are such division operations common enough to benefit from compiler generated code?

We explored and prototyped the approach of adding a functions to compiler-rt in a different PR. We discovered multiple issues.

Set limit to 64 bit for ARM and x86_32

Clarify that x86_32 cannot lower division with more than 64 bits.

Fix description of expand-div-rem-bits argument
Use ConstantInt::get

Abandoned in favor of https://reviews.llvm.org/D130076

I'm abandoning this approach in favor of https://reviews.llvm.org/D130076

Instead of linking to libbitint, I instead created a backend pass to transform div/rem instructions: https://reviews.llvm.org/D130076

Remove pass from pass pipeline (will be added in next PR)

Thanks for the review! I updated the PR to reflect your comments.

Implement review comments

In D126644#3645177, @nikic wrote:

I'm a bit confused, where in the code does this actually create functions? It doesn't look like expandDivision() does that, and I can't spot anything else creating __llvm_udivXXX either.

Ping, how can I get review/approval for this PR? Should I add somebody else as reviewer?

Merged

With the work around constant expressions (e.g. https://reviews.llvm.org/rG941c8e0ea50b), I might not need to handle div/mod constants anymore. For that reason, I would at least land this PR without support for transforming constant expressions.

Use IntegerDivison.cpp
Do not emit separate functions
Turn into FunctionPass

In D126644#3582347, @arsenm wrote:

Another problem is this will miss divisions embedded in constant expressions

In D126644#3581484, @arsenm wrote:

Actually, how is this really different from lib/Transforms/Utils/IntegerDivision.cpp? This just goes to 128 but the existing code goes up to 64. Can you merge them, or at least move the implementation bodies to the same place?

In D126644#3580139, @efriedma wrote:

My primary concern here is that we're unconditionally iterating over every instruction in the module, even if the pass has nothing to do. In the grand scheme of things, it isn't *that* expensive, but it's still not cheap, and we seem to have grown a number of passes doing similar walks. Can we merge this with some other walk over the module?

Thanks for the quick review!

Used Optional
Added entry to release notes
Modifed help text according to suggestion
Added comment to LANGOPT that it will be removed in the future

I split the introduction of -fexperimental-max-bitint-width into https://reviews.llvm.org/D127287 because there is still discussion about the bitint libraries in the backend PRs.

I have tried to circumvent various issues around changing compiler-rt by synthesizing the division function directly in LLVM: https://reviews.llvm.org/D126644

There is some push back on the gcc mailing list both on requiring an extra word to enable Knuth [0]
and generally defining the libgcc function before _BitInt is implemented in gcc [1].
There are also concerns about whether this would be consistent with the ABI,
which is isn't finalized on all targets yet.

In D120327#3492053, @efriedma wrote:

Instead of putting the __divmodei5/__udivmodei5 routines in compiler-rt, can we just make the backend embed them into the object file?

unsigned int -> unsigned
Fix memory leaks in unit tests

Note for self: Also broke armv8 here: https://lab.llvm.org/buildbot/#/builders/178/builds/1902

In D120327#3438559, @RKSimon wrote:

@mgehre-amd Please can you take a look at the buildbots breaks: https://lab.llvm.org/buildbot/#/builders/93/builds/8214

In D120327#3437830, @FreddyYe wrote:

LGTM. Will you update the backend libcall?

@howard.hinnant, as Code Owner, do you know a good person to review/approve this? @programmerjake would like a second opinion.

Require 'a' to be a large integer with n + 1 words; update tests accordingly

• Remove diagnostics for float-to-bitint
• Remove lexer changes (will be another PR)
• Add -fexperimental-max-bitint-width=N

Implement a basic long division
Make it work on big endian machines
Only provide

void __divmodei5(su_int *quo, su_int *rem, su_int *a, su_int *b,
                               unsigned int words)

and

void __udivmodei5(su_int *quo, su_int *rem, const su_int *a,
                                const su_int *b, unsigned int words)

In that case I will go for a subtract-shift implementation because Knuth seem to require multiple temporary buffers, and it would be odd to expose all of them through the API.
It also means that I likely need to modify the LLVM backend to emit calls to divmodei5 and not divei4 or __modei4.

I opened the review for the clang changes (including linking bitint by default) here: https://reviews.llvm.org/D122234

Thanks a lot!

Hey, yes it's still near the top of my TODO list. Had a few other things to do, but I'm planning to come back to this.

LGTM

@LuoYuanke, thank you very much for the review and comments!

Rename to ExpandExtIntRes_DIVREM
Add nounwind

Add a test case for aarch64 (showing big endian)