Skip to content

Commit 3bcccb6

Browse files
authored
[Reassociate] Drop weight reduction to fix issue 91417 (llvm#91469)
See the following case: https://alive2.llvm.org/ce/z/A-fBki ``` define i3 @src(i3 %0) { %2 = mul i3 %0, %0 %3 = mul i3 %2, %0 %4 = mul i3 %3, %0 %5 = mul nsw i3 %4, %0 ret i3 %5 } define i3 @tgt(i3 %0) { %2 = mul i3 %0, %0 %5 = mul nsw i3 %2, %0 ret i3 %5 } ``` llvm@d7aeefe introduced weight reduction during weights combination of the same operand. As the weight of `%0` changes from 5 to 3, the nsw flag in `%5` should be dropped. However, the nsw flag isn't cleared by `RewriteExprTree` since `%5 = mul nsw i3 %0, %4` is not included in the range of `[ExpressionChangedStart, ExpressionChangedEnd)`. ``` Calculated Rank[] = 3 Combine negations for: %2 = mul i3 %0, %0 Calculated Rank[] = 4 Combine negations for: %3 = mul i3 %0, %2 Calculated Rank[] = 5 Combine negations for: %4 = mul i3 %0, %3 Calculated Rank[] = 6 Combine negations for: %5 = mul nsw i3 %0, %4 LINEARIZE: %5 = mul nsw i3 %0, %4 OPERAND: i3 %0 (1) ADD USES LEAF: i3 %0 (1) OPERAND: %4 = mul i3 %0, %3 (1) DIRECT ADD: %4 = mul i3 %0, %3 (1) OPERAND: i3 %0 (1) OPERAND: %3 = mul i3 %0, %2 (1) DIRECT ADD: %3 = mul i3 %0, %2 (1) OPERAND: i3 %0 (1) OPERAND: %2 = mul i3 %0, %0 (1) DIRECT ADD: %2 = mul i3 %0, %0 (1) OPERAND: i3 %0 (1) OPERAND: i3 %0 (1) RAIn: mul i3 [ %0, #3] [ %0, #3] [ %0, #3] RAOut: mul i3 [ %0, #3] [ %0, #3] [ %0, #3] RAOut after CSE reorder: mul i3 [ %0, #3] [ %0, #3] [ %0, #3] RA: %5 = mul nsw i3 %0, %4 TO: %5 = mul nsw i3 %4, %0 RA: %4 = mul i3 %0, %3 TO: %4 = mul i3 %0, %0 ``` The best way to fix this is to inform `RewriteExprTree` to clear flags of the whole expr tree when weight reduction happens. But I find that weight reduction based on Carmichael number never happens in practice. See the coverage result https://dtcxzyw.github.io/llvm-opt-benchmark/coverage/home/dtcxzyw/llvm-project/llvm/lib/Transforms/Scalar/Reassociate.cpp.html#L323 I think it would be better to drop `IncorporateWeight`. Fixes llvm#91417
1 parent 971f1aa commit 3bcccb6

File tree

2 files changed

+126
-173
lines changed

2 files changed

+126
-173
lines changed

llvm/lib/Transforms/Scalar/Reassociate.cpp

Lines changed: 1 addition & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -302,97 +302,6 @@ static BinaryOperator *LowerNegateToMultiply(Instruction *Neg) {
302302
return Res;
303303
}
304304

305-
/// Returns k such that lambda(2^Bitwidth) = 2^k, where lambda is the Carmichael
306-
/// function. This means that x^(2^k) === 1 mod 2^Bitwidth for
307-
/// every odd x, i.e. x^(2^k) = 1 for every odd x in Bitwidth-bit arithmetic.
308-
/// Note that 0 <= k < Bitwidth, and if Bitwidth > 3 then x^(2^k) = 0 for every
309-
/// even x in Bitwidth-bit arithmetic.
310-
static unsigned CarmichaelShift(unsigned Bitwidth) {
311-
if (Bitwidth < 3)
312-
return Bitwidth - 1;
313-
return Bitwidth - 2;
314-
}
315-
316-
/// Add the extra weight 'RHS' to the existing weight 'LHS',
317-
/// reducing the combined weight using any special properties of the operation.
318-
/// The existing weight LHS represents the computation X op X op ... op X where
319-
/// X occurs LHS times. The combined weight represents X op X op ... op X with
320-
/// X occurring LHS + RHS times. If op is "Xor" for example then the combined
321-
/// operation is equivalent to X if LHS + RHS is odd, or 0 if LHS + RHS is even;
322-
/// the routine returns 1 in LHS in the first case, and 0 in LHS in the second.
323-
static void IncorporateWeight(APInt &LHS, const APInt &RHS, unsigned Opcode) {
324-
// If we were working with infinite precision arithmetic then the combined
325-
// weight would be LHS + RHS. But we are using finite precision arithmetic,
326-
// and the APInt sum LHS + RHS may not be correct if it wraps (it is correct
327-
// for nilpotent operations and addition, but not for idempotent operations
328-
// and multiplication), so it is important to correctly reduce the combined
329-
// weight back into range if wrapping would be wrong.
330-
331-
// If RHS is zero then the weight didn't change.
332-
if (RHS.isMinValue())
333-
return;
334-
// If LHS is zero then the combined weight is RHS.
335-
if (LHS.isMinValue()) {
336-
LHS = RHS;
337-
return;
338-
}
339-
// From this point on we know that neither LHS nor RHS is zero.
340-
341-
if (Instruction::isIdempotent(Opcode)) {
342-
// Idempotent means X op X === X, so any non-zero weight is equivalent to a
343-
// weight of 1. Keeping weights at zero or one also means that wrapping is
344-
// not a problem.
345-
assert(LHS == 1 && RHS == 1 && "Weights not reduced!");
346-
return; // Return a weight of 1.
347-
}
348-
if (Instruction::isNilpotent(Opcode)) {
349-
// Nilpotent means X op X === 0, so reduce weights modulo 2.
350-
assert(LHS == 1 && RHS == 1 && "Weights not reduced!");
351-
LHS = 0; // 1 + 1 === 0 modulo 2.
352-
return;
353-
}
354-
if (Opcode == Instruction::Add || Opcode == Instruction::FAdd) {
355-
// TODO: Reduce the weight by exploiting nsw/nuw?
356-
LHS += RHS;
357-
return;
358-
}
359-
360-
assert((Opcode == Instruction::Mul || Opcode == Instruction::FMul) &&
361-
"Unknown associative operation!");
362-
unsigned Bitwidth = LHS.getBitWidth();
363-
// If CM is the Carmichael number then a weight W satisfying W >= CM+Bitwidth
364-
// can be replaced with W-CM. That's because x^W=x^(W-CM) for every Bitwidth
365-
// bit number x, since either x is odd in which case x^CM = 1, or x is even in
366-
// which case both x^W and x^(W - CM) are zero. By subtracting off multiples
367-
// of CM like this weights can always be reduced to the range [0, CM+Bitwidth)
368-
// which by a happy accident means that they can always be represented using
369-
// Bitwidth bits.
370-
// TODO: Reduce the weight by exploiting nsw/nuw? (Could do much better than
371-
// the Carmichael number).
372-
if (Bitwidth > 3) {
373-
/// CM - The value of Carmichael's lambda function.
374-
APInt CM = APInt::getOneBitSet(Bitwidth, CarmichaelShift(Bitwidth));
375-
// Any weight W >= Threshold can be replaced with W - CM.
376-
APInt Threshold = CM + Bitwidth;
377-
assert(LHS.ult(Threshold) && RHS.ult(Threshold) && "Weights not reduced!");
378-
// For Bitwidth 4 or more the following sum does not overflow.
379-
LHS += RHS;
380-
while (LHS.uge(Threshold))
381-
LHS -= CM;
382-
} else {
383-
// To avoid problems with overflow do everything the same as above but using
384-
// a larger type.
385-
unsigned CM = 1U << CarmichaelShift(Bitwidth);
386-
unsigned Threshold = CM + Bitwidth;
387-
assert(LHS.getZExtValue() < Threshold && RHS.getZExtValue() < Threshold &&
388-
"Weights not reduced!");
389-
unsigned Total = LHS.getZExtValue() + RHS.getZExtValue();
390-
while (Total >= Threshold)
391-
Total -= CM;
392-
LHS = Total;
393-
}
394-
}
395-
396305
using RepeatedValue = std::pair<Value*, APInt>;
397306

398307
/// Given an associative binary expression, return the leaf
@@ -562,26 +471,7 @@ static bool LinearizeExprTree(Instruction *I,
562471
"In leaf map but not visited!");
563472

564473
// Update the number of paths to the leaf.
565-
IncorporateWeight(It->second, Weight, Opcode);
566-
567-
#if 0 // TODO: Re-enable once PR13021 is fixed.
568-
// The leaf already has one use from inside the expression. As we want
569-
// exactly one such use, drop this new use of the leaf.
570-
assert(!Op->hasOneUse() && "Only one use, but we got here twice!");
571-
I->setOperand(OpIdx, UndefValue::get(I->getType()));
572-
Changed = true;
573-
574-
// If the leaf is a binary operation of the right kind and we now see
575-
// that its multiple original uses were in fact all by nodes belonging
576-
// to the expression, then no longer consider it to be a leaf and add
577-
// its operands to the expression.
578-
if (BinaryOperator *BO = isReassociableOp(Op, Opcode)) {
579-
LLVM_DEBUG(dbgs() << "UNLEAF: " << *Op << " (" << It->second << ")\n");
580-
Worklist.push_back(std::make_pair(BO, It->second));
581-
Leaves.erase(It);
582-
continue;
583-
}
584-
#endif
474+
It->second += Weight;
585475

586476
// If we still have uses that are not accounted for by the expression
587477
// then it is not safe to modify the value.

0 commit comments

Comments
 (0)