Paths
| Differential D56057
[X86] Individually simplify both operands of PMULDQ/PMULUDQ using the other entry point of SimplifyDemandedBits that allows the one use check of the root node to be suppressed. AbandonedPublic Authored by craig.topper on Dec 22 2018, 9:57 PM.
Details Summary This entry point takes special care to avoid completely replacing the root node if it has multiple uses. Instead it will just use UpdateNodeOperands to only update the PMULDQ/PMULUDQ node. Not sure if that can create additional instructions in some cases. This allows masking and sign_extend_inreg opcodes to be removed from the input of these operations. Fixes PR40142.
Diff Detail
Event TimelineComment Actions Its really annoying that we have to do this but I can't see any other way.
Comment Actions I am a little suspicious about these changes to the test results.
Comment Actions
I think you'e referring to pmul.ll? rL347181 just replaced psrad $31 with pcmpgt. It changed the register allocation quite a bit which made the diff look large. But it didn't change the number of instructions other than copies and xors.
I'll take another look.
This revision is now accepted and ready to land.Dec 23 2018, 2:22 PM Comment Actions Looking at the vector-reduce-mul.ll changes, I don't think this patch is valid. If the first node has multiple uses we can't propagate the input demanded bits to the next operation down. Comment Actions
Yeah, I checked it. It's definitely broken. Although, @RKSimon, that is a perfect example of what I was saying, not checking tests. You almost accepted a patch that broke multivector 64-bit multiplication completely. I transpiled the assembly from the test results and ran a diff, and sure enough, broken. .text
.globl test_v4i64
// long long test_v4i64(i64x2 xmm0, i64x2 xmm1)
test_v4i64:
#ifndef NEW
movdqa %xmm0, %xmm2
psrlq $32, %xmm2
pmuludq %xmm1, %xmm2
movdqa %xmm1, %xmm3
psrlq $32, %xmm3
pmuludq %xmm0, %xmm3
paddq %xmm2, %xmm3
psllq $32, %xmm3
#endif
pmuludq %xmm1, %xmm0
#ifndef NEW
paddq %xmm3, %xmm0
#endif
pshufd $78, %xmm0, %xmm1 # xmm1 = xmm0[2,3,0,1]
movdqa %xmm0, %xmm2
psrlq $32, %xmm2
pmuludq %xmm1, %xmm2
movdqa %xmm0, %xmm3
psrldq $12, %xmm3
pmuludq %xmm0, %xmm3
paddq %xmm2, %xmm3
psllq $32, %xmm3
pmuludq %xmm1, %xmm0
paddq %xmm3, %xmm0
movq %xmm0, %rax
retq#include <stdio.h>
typedef long long i64x2 __attribute__((vector_size(16)));
extern long long test_v4i64(i64x2 val1, i64x2 val2); // GCC passes a pointer if I emulate an i64x4
int main(void) {
i64x2 test = { 1234567812345678LL, 2345678923456789LL },
test2 = { 1314151617181910LL, 9694959695969798LL };
printf ("%lld\n", test_v4i64(test, test2));
}Compiled with GCC to avoid any LLVM-specific issues. -UNEW: -7524135448842347520 Comment Actions The really concerning thing is that this patch does the exact same thing that's done by simplifyI24 in AMDGPUISelLowering.cpp. I assume Simon like myself assumed that established infrastructure like that was doing the right thing and didn't question it too much. Comment Actions
Ok. Well, unfortunately, that too is probably broken. I would test it myself, but I only have integrated on a Sandy Bridge laptop. The good thing is that we are catching it here. It could've been a lot worse. If this got into x86 world, it would go from a visual glitch to someone getting a bill for $4,294,967,295 because of an arithmetic bug in the compiler. Sorry if I came off sounding like a jerk, by the way. I understand that there are a lot of patches that come in, and checking the thousands of tests is time consuming, and I don't expect every single test to be read line for line. I was just a little shocked when I saw this patch, with hundreds of lines removed from the test results, pass review without question, even after I had already expressed my concerns with it. Comment Actions I'm sorry about that, I focussed on checking the earlier (and by looks of it easier) test cases - SimplifyDemandedBits patches tend to cause a lot of changes with a lot of interesting cases where it can throw away a lot of code. So I was expecting some changes like that and didn't look at them more critically. The splitting version of SimplifyDemandedBits now scares me - I think the issue is that the TLO.Old.hasOneUse() code doesn't take into account that it might have gone through several other ops that had multiple uses but I haven't checked this yet. Comment Actions The AssumeSingleUse code will only allow the root to have multiple uses. The issue is that if the root has multiple uses, the most you should be allowed to do is decide if the root node is necessary for the instruction that is using it and replace it with one of it operands in this use. No new nodes should be created. You cannot recurse into SimplifyDemandedBits any further. You can only use computeKnownBits to make the decision. This is analogous to the code in InstCombines’s SimplifyMultipleUseDemandedBits.
Revision Contents
Diff 179450 lib/Target/X86/X86ISelLowering.cpp
test/CodeGen/X86/avx2-intrinsics-fast-isel.ll
test/CodeGen/X86/avx512-intrinsics-fast-isel.ll
test/CodeGen/X86/pmul.ll
test/CodeGen/X86/sse2-intrinsics-fast-isel.ll
test/CodeGen/X86/sse41-intrinsics-fast-isel.ll
test/CodeGen/X86/vector-mul.ll
test/CodeGen/X86/vector-reduce-mul-widen.ll
test/CodeGen/X86/vector-reduce-mul.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Add a comment explaining what we're we having to do here.