If both operands of a rem instruction are left shifts of the same value, this can be simplified to 0 or the first operand
|130 ms||x64 debian > Flang.Driver::target-cpu-features.f90|
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/flang-new --target=aarch64-linux-gnu -mcpu=cortex-a57 -c /var/lib/buildkite-agent/builds/llvm-project/flang/test/Driver/target-cpu-features.f90 -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/flang/test/Driver/target-cpu-features.f90 -check-prefix=CHECK-A57
Here are some alive tests which support the transforms:
Nice improvement @MattDevereau! I just had a few minor comments ...
Perhaps you can create a temp variable here to reuse the logic, i.e.
bool NoWrap = (IsSigned && Q.IIQ.hasNoSignedWrap(Shift)) || (!IsSigned && Q.IIQ.hasNoUnsignedWrap(Shift)); if (S1 >= S2 && NoWrap) return Constant::getNullValue(Shift->getType()); if (NoWrap) return Op0;
Could you leave comments on the negative tests explaining why they fail? If it makes it easier you could put all the negative tests together?
Is it worth having a negative test for srem with nuw shifts as well? We shouldn't do any simplification in that case either.
I'm not sure this suggestion is correct, they key thing here being Shift = cast<OverflowingBinaryOperator>(Op1); on line 1010 which changes Shift to be a cast of Op1 instead of Op0 on which the nsw/nuw flags are checked, which is because the larger shift value needs its flags checking. If we went ahead with this, in this example (https://alive2.llvm.org/ce/z/YiFELt) NoWrap would evaluate to true while the transform is not viable.
Sure thing, should be simple enough
I personally think the current tests are ok since we have one test for nsw which is testing the presence of nsw, and one which tests for it's absence and other flags aren't particularly relevant. These tests are quite compact though so I suppose there's no harm in adding extra cases.
Instead of a new helper maybe this should be in simplifyRem which already appears to have:
// (X << Y) % X -> 0 if (Q.IIQ.UseInstrInfo && ((Opcode == Instruction::SRem && match(Op0, m_NSWShl(m_Specific(Op1), m_Value()))) || (Opcode == Instruction::URem && match(Op0, m_NUWShl(m_Specific(Op1), m_Value()))))) return Constant::getNullValue(Op0->getType());
This is really just a superset of that case, so maybe expanding the existing logic would be simpler?
Maybe add a TODO for the more generalized case of (rem (mul nsw/nuw X, C1), (mul nsw/nuw X, C2) if C1 % C2 == 0 -> 0 We seem to be missing it: https://godbolt.org/z/fzxb4sxb5
Sure, I was just thinking that specifically we don't want to perform the simplification for any of nsw or nuw. They both mark the instruction as not wrapping, but only the signed version applies here. It's just because your logic accepts both nsw and nuw flags as valid, but *only if* the signedness matches the instruction. I was hoping to defend against someone accidentally rewriting your code in future and losing the signedness checks.
Inlined unnecessary helper function
Added comments to negative tests
Added more incorrect flag tests
Note that this patch will likely be dropped in favour of https://reviews.llvm.org/D143014