Details
Diff Detail
Event Timeline
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 3103–3104 | You're ignoring this comment - consult simplifyDivRem inside InstructionSimplify.cpp which shows the issue | |
IIRC last time I started looking at this a load of tests failed in other targets, including some reduced test cases that needed fixing.
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 3103–3104 | Seems like we don't even need the X /, % 1 -> X fold here. With or without - tests are not affected, this was folded even before this patch. | |
| test/CodeGen/X86/pr38539.ll | ||
|---|---|---|
| 6 | @craig.topper will be able to confirm but it might be that you need to (locally) revert the fix for PR38539, apply your DAGCombiner.cpp patch and then run bugpoint to reduce the original fuzz code again: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8882 | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 3103 | Please don't remove this comment without actually fixing the underlying problem (in a separate patch).
Here are test cases you can add/use to confirm the difference between llc and opt for the boolean patterns: define i1 @bool_udiv(i1 %x, i1 %y) {
%r = udiv i1 %x, %y
ret i1 %r
}
define i1 @bool_sdiv(i1 %x, i1 %y) {
%r = sdiv i1 %x, %y
ret i1 %r
}
define i1 @bool_urem(i1 %x, i1 %y) {
%r = urem i1 %x, %y
ret i1 %r
}
define i1 @bool_srem(i1 %x, i1 %y) {
%r = srem i1 %x, %y
ret i1 %r
}
define <4 x i1> @boolvec_udiv(<4 x i1> %x, <4 x i1> %y) {
%r = udiv <4 x i1> %x, %y
ret <4 x i1> %r
}
define <4 x i1> @boolvec_sdiv(<4 x i1> %x, <4 x i1> %y) {
%r = sdiv <4 x i1> %x, %y
ret <4 x i1> %r
}
define <4 x i1> @boolvec_urem(<4 x i1> %x, <4 x i1> %y) {
%r = urem <4 x i1> %x, %y
ret <4 x i1> %r
}
define <4 x i1> @boolvec_srem(<4 x i1> %x, <4 x i1> %y) {
%r = srem <4 x i1> %x, %y
ret <4 x i1> %r
} | |
| test/CodeGen/X86/pr38539.ll | ||
|---|---|---|
| 6 | I reverted https://reviews.llvm.org/rL339945 and I ran llvm-lit on pr38539.ll - test passed... (flags-copy-lowering.mir failed). Something else between that commit and top of trunk must fix it too. So I think we should either remove test file or modify it with no worries. | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 3103 | I will revert that comment removal.. Anyway, I see // fold (sdiv X, 1) -> X if (N1C && N1C->isOne()) return N0; in visitS(U)DIV so.. do you suggest to unify it and move it here? | |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 3103 | Yes, I think that's a good change - reduce code duplication and more efficient (for urem/srem). You can do that cleanup first as an NFC patch. Then, add the functionality for bool types as a follow-up. (Both changes are independent of this patch.) | |
Please don't remove this comment without actually fixing the underlying problem (in a separate patch).
I think it would be better to consolidate the existing code here for 2 reasons:
Here are test cases you can add/use to confirm the difference between llc and opt for the boolean patterns:
define i1 @bool_udiv(i1 %x, i1 %y) { %r = udiv i1 %x, %y ret i1 %r } define i1 @bool_sdiv(i1 %x, i1 %y) { %r = sdiv i1 %x, %y ret i1 %r } define i1 @bool_urem(i1 %x, i1 %y) { %r = urem i1 %x, %y ret i1 %r } define i1 @bool_srem(i1 %x, i1 %y) { %r = srem i1 %x, %y ret i1 %r } define <4 x i1> @boolvec_udiv(<4 x i1> %x, <4 x i1> %y) { %r = udiv <4 x i1> %x, %y ret <4 x i1> %r } define <4 x i1> @boolvec_sdiv(<4 x i1> %x, <4 x i1> %y) { %r = sdiv <4 x i1> %x, %y ret <4 x i1> %r } define <4 x i1> @boolvec_urem(<4 x i1> %x, <4 x i1> %y) { %r = urem <4 x i1> %x, %y ret <4 x i1> %r } define <4 x i1> @boolvec_srem(<4 x i1> %x, <4 x i1> %y) { %r = srem <4 x i1> %x, %y ret <4 x i1> %r }