[APInt] Use a std::move() to avoid a copy in the loop in multiplicativeInverse.

[topperc]

This allows the subtract to reuse the storage of T. T will be assigned over by the condition on the next iteration. I think assigning over a moved from value should be ok.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Craig Topper (topperc)

Changes

This allows the subtract to reuse the storage of T. T will be assigned over by the condition on the next iteration. I think assigning over a moved from value should be ok.

---

Full diff: https://github.com/llvm/llvm-project/pull/87655.diff

1 Files Affected:

• (modified) llvm/lib/Support/APInt.cpp (+1-1)

diff --git a/llvm/lib/Support/APInt.cpp b/llvm/lib/Support/APInt.cpp
index 224ea0924f0aaa..8825025ec32134 100644
--- a/llvm/lib/Support/APInt.cpp
+++ b/llvm/lib/Support/APInt.cpp
@@ -1249,7 +1249,7 @@ APInt APInt::multiplicativeInverse() const {
   APInt Factor = *this;
   APInt T;
   while (!(T = *this * Factor).isOne())
-    Factor *= 2 - T;
+    Factor *= 2 - std::move(T);
   return Factor;
 }
 

[jayfoad]

No objection from me, but this is slightly beyond my knowledge of C++. I always wonder, why can't the compiler work this out for itself?

[jayfoad]

Incidentally I was disappointed that C++ didn't let me write:

  while (APInt T = *this * Factor; !T.isOne())
    Factor *= 2 - T;

Perhaps this would have allowed the compiler to do the move, since T goes out of scope at the end of the body of the while?

[topperc]

Incidentally I was disappointed that C++ didn't let me write:

  while (APInt T = *this * Factor; !T.isOne())
    Factor *= 2 - T;

Perhaps this would have allowed the compiler to do the move, since T goes out of scope at the end of the body of the while?

I think that didn't make it into the standard because you can write

  for (APInt T = *this * Factor; !T.isOne();)
    Factor *= 2 - T;

[topperc]

Incidentally I was disappointed that C++ didn't let me write:

  while (APInt T = *this * Factor; !T.isOne())
    Factor *= 2 - T;

Perhaps this would have allowed the compiler to do the move, since T goes out of scope at the end of the body of the while?

I think that didn't make it into the standard because you can write

  for (APInt T = *this * Factor; !T.isOne();)
    Factor *= 2 - T;

Err that's why while doesn't have an initializer statement, but you'd still need to update T on each iteration. So the for I posted doesn't work.

[jayfoad]

I guess I didn't explain what I intended with that invalid while loop syntax. Let me try again with valid syntax:

  while (true) {
    APInt T = *this * Factor;
    if (T.isOne())
      break;
    Factor *= 2 - T;
  }

Since T goes out of scope at the end of every iteration, would you still need the std::move, or could the compiler do it for you?

[topperc]

I guess I didn't explain what I intended with that invalid while loop syntax. Let me try again with valid syntax:

  while (true) {
    APInt T = *this * Factor;
    if (T.isOne())
      break;
    Factor *= 2 - T;
  }

Since T goes out of scope at the end of every iteration, would you still need the std::move, or could the compiler do it for you?

It looks like that's still invoking the copy constructor when it creates b for this operator overload inline APInt operator-(uint64_t LHS, APInt b). That copy constructor does a heap allocation and a memcpy if the bit width is greater than 64.

[efriedma-quic]

This is still allocating every iteration. If you want to completely avoid that, you'd have to avoid freeing the allocation in T. For example:

while (1) {
  T = *this;  // APInt has a special-case to avoid reallocating here.
  T *= Factor;
  if (T.isOne())
    break;
  T.negate();
  T += 2;
  Factor *= T;
}

Not sure how much we want to trade off readability here, though. The current version might be good enough.

I always wonder, why can't the compiler work this out for itself?

In C++, the compiler is required to emit the operations as written, barring a few special cases where copy-elision is specifically allowed. There are different tradeoffs in other rules (e.g. this example just works in Rust, but Rust doesn't allow overriding the move construction/assignment operators).

[topperc]

This is still allocating every iteration. If you want to completely avoid that, you'd have to avoid freeing the allocation in T. For example:

while (1) {
  T = *this;  // APInt has a special-case to avoid reallocating here.
  T *= Factor;
  if (T.isOne())
    break;
  T.negate();
  T += 2;
  Factor *= T;
}

Not sure how much we want to trade off readability here, though. The current version might be good enough.

operator*= for APInt still does a new allocation doesn't it?

APInt &APInt::operator*=(const APInt &RHS) {
  *this = *this * RHS;
  return *this;
}

[efriedma-quic]

operator*= for APInt still does a new allocation doesn't it?

Umm... yes, I guess. Maybe we should consider adding void APInt::mla(const APInt &a, const APInt &b), which wouldn't require an allocation.

[topperc]

operator*= for APInt still does a new allocation doesn't it?

Umm... yes, I guess. Maybe we should consider adding void APInt::mla(const APInt &a, const APInt &b), which wouldn't require an allocation.

I guess you mean using the addend as the destination buffer for tcMultiply?

Well I guess we'd need to change tcMultiply to not zero the dst first.