[llvm] r207338 - DAGCombiner: Simplify code a bit, make more transforms work with vectors.
Benjamin Kramer
benny.kra at gmail.com
Fri Jul 11 03:38:13 PDT 2014
On Fri, Jul 11, 2014 at 12:05 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> Hi Benjamin,
>
> Attached is the rebased patch.
>
> I’ve kept your if/else construction for the setting of the mask, but I can
> apply the array of array approach of my initial proposal if you prefer.
I like the if/else approach more. The code has enough obscurity
already, a small bit of duplication doesn't hurt.
>
> Note that like the original patch, this patch also corrects the low <-> high
> inversion.
> This result in a diff on idiv test case for SSE variant that I believe is
> correct.
> Indeed, there is no reason that we see the fix up of pmuludq in that test,
> since we are just reading the low bits!
>
> Is it OK to commit?
LGTM. Thanks a lot for fixing this.
- Ben
>
> Thanks,
> -Quentin
>
>
> On Jul 10, 2014, at 10:59 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>
>
> On 10.07.2014, at 19:46, Quentin Colombet <qcolombet at apple.com> wrote:
>
> Hi Benjamin,
>
> I have narrowed down a failure of the factor test case (from the llvm
> test-suite: SingleSource/UnitTests/SignlessTypes) to this commit.
>
> The failure happens at Os on a haswell machine running macosx.
>
> I have reduced the problem to the attached test.ll and I proposed to fix
> that with the attached patch.
> Basically the masks for the shuffle are wrong for vectors with 8 elements.
>
> ** To Reproduce **
>
> Basically this test takes 8 numbers: 8, 9, …, 15 and divide each of them by
> 7 and print the result.
>
> On a haswell machine
> llc -mattr=-avx2 test.ll -o ok.s
> llc -mattr=+avx2 test.ll -o ko.s
>
> clang ok.s -arch x86_64 -o ok.out
> clang ko.s -arch x86_64 -march core-avx2 -o ko.out
>
> ./ok.out
> 1
> 1
> 1
> 1
> 1
> 1
> 2
> 2
>
>
> ./ko.out
> 1
> 1
> 1
> 1
> 1
> 1
> 1 <-- 14/7 == 1!!!
> 2
>
> ** Feedback Needed **
>
> Could you review the attached patch?
>
> Moreover, because of this patch, vector-idiv.ll is failing. To me the output
> seems reasonable but could you take a look?
> I’ll update the patch to match the new pattern if they are fine.
>
>
> Your patch is correct, and vector-idiv.ll was buggy. I think this bug is
> already fixed with r212611 (the shufflemasks I had in place for AVX2 were
> completely bogus, I should've noticed this earlier). However, I like the
> effort that you put into documenting the lowering with verbose comments.
> Would you mind rebasing them on top of r212611?
>
> Really sorry that you had to spent so much time on this silly oversight of
> mine.
> - Ben
>
>
> Thanks,
> -Quentin
> <lower_mullohi.patch><test.ll>
>
> This commit (and more generally the custom lowering of MUL_LOHI)
>
> On May 2, 2014, at 5:42 AM, Benjamin Kramer <benny.kra at gmail.com> wrote:
>
>
> On 02.05.2014, at 13:48, Patrik Hägglund H <patrik.h.hagglund at ericsson.com>
> wrote:
>
> Hi Benjamin,
>
> This commit is causing a regression for an llvm-stress test, as shown below.
>
>
> r207835 fixes the crash. Thanks for the test case!
>
> - Ben
>
>
> /Patrik Hägglund
>
> bin/llvm-stress -size 185 -seed 7564 | bin/llc -march=x86-64 -mcpu=corei7 -o
> /dev/null
>
> llc: ../lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5728: void
> llvm::SelectionDAG::ReplaceAllUsesWith(llvm::SDNode *, llvm::SDNode *):
> Assertion `(!From->hasAnyUseOfValue(i) || From->getValueType(i) ==
> To->getValueType(i)) && "Cannot use this version of ReplaceAllUsesWith!"'
> failed.
> 0 llc 0x00000000010b5a55 llvm::sys::PrintStackTrace(_IO_FILE*)
> + 37
> 1 llc 0x00000000010b5e93
> 2 libpthread.so.0 0x00007f7d65bcb7c0
> 3 libc.so.6 0x00007f7d64ecfb35 gsignal + 53
> 4 libc.so.6 0x00007f7d64ed1111 abort + 385
> 5 libc.so.6 0x00007f7d64ec89f0 __assert_fail + 240
> 6 llc 0x0000000000f3a524
> 7 llc 0x0000000000ec4e45
> llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AliasAnalysis&,
> llvm::CodeGenOpt::Level) + 1573
> 8 llc 0x0000000000fad035
> llvm::SelectionDAGISel::CodeGenAndEmitDAG() + 901
> 9 llc 0x0000000000fac368
> llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) + 6504
> 10 llc 0x0000000000fa9c64
> llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) + 1332
> 11 llc 0x0000000000c3261c
> llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 124
> 12 llc 0x0000000000e1f20a
> llvm::FPPassManager::runOnFunction(llvm::Function&) + 362
> 13 llc 0x0000000000e1f49b
> llvm::FPPassManager::runOnModule(llvm::Module&) + 43
> 14 llc 0x0000000000e1fa37
> llvm::legacy::PassManagerImpl::run(llvm::Module&) + 999
> 15 llc 0x000000000056d835 main + 6549
> 16 libc.so.6 0x00007f7d64ebbc16 __libc_start_main + 230
> 17 llc 0x000000000056bdb9
> Stack dump:
> 0. Program arguments: bin/llc -march=x86-64 -mcpu=corei7 -o /dev/null
> 1. Running pass 'Function Pass Manager' on module '<stdin>'.
> 2. Running pass 'X86 DAG->DAG Instruction Selection' on function
> '@autogen_SD7564'
> Abort
>
>
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu
> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Benjamin Kramer
> Sent: den 27 april 2014 01:10
> To: llvm-commits at cs.uiuc.edu
> Subject: [llvm] r207338 - DAGCombiner: Simplify code a bit, make more
> transforms work with vectors.
>
> Author: d0k
> Date: Sat Apr 26 18:09:49 2014
> New Revision: 207338
>
> URL: http://llvm.org/viewvc/llvm-project?rev=207338&view=rev
> Log:
> DAGCombiner: Simplify code a bit, make more transforms work with vectors.
>
> Modified:
> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> llvm/trunk/test/CodeGen/X86/vector-idiv.ll
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=207338&r1=207337&r2=207338&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Sat Apr 26 18:09:49
> 2014
> @@ -644,8 +644,13 @@ static ConstantSDNode *isConstOrConstSpl
> if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N))
> return CN;
>
> - if (BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(N))
> - return BV->getConstantSplatValue();
> + if (BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(N)) {
> + ConstantSDNode *CN = BV->getConstantSplatValue();
> +
> + // BuildVectors can truncate their operands. Ignore that case here.
> + if (CN && CN->getValueType(0) == N.getValueType().getScalarType())
> + return CN;
> + }
>
> return nullptr;
> }
> @@ -1957,8 +1962,8 @@ SDValue DAGCombiner::visitMUL(SDNode *N)
> SDValue DAGCombiner::visitSDIV(SDNode *N) {
> SDValue N0 = N->getOperand(0);
> SDValue N1 = N->getOperand(1);
> - ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0.getNode());
> - ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1.getNode());
> + ConstantSDNode *N0C = isConstOrConstSplat(N0);
> + ConstantSDNode *N1C = isConstOrConstSplat(N1);
> EVT VT = N->getValueType(0);
>
> // fold vector ops
> @@ -1985,25 +1990,15 @@ SDValue DAGCombiner::visitSDIV(SDNode *N
> N0, N1);
> }
>
> - const APInt *Divisor = nullptr;
> - if (N1C) {
> - Divisor = &N1C->getAPIntValue();
> - } else if (N1.getValueType().isVector() &&
> - N1->getOpcode() == ISD::BUILD_VECTOR) {
> - BuildVectorSDNode *BV = cast<BuildVectorSDNode>(N->getOperand(1));
> - if (ConstantSDNode *C = BV->getConstantSplatValue())
> - Divisor = &C->getAPIntValue();
> - }
> -
> // fold (sdiv X, pow2) -> simple ops after legalize
> - if (Divisor && !!*Divisor &&
> - (Divisor->isPowerOf2() || (-*Divisor).isPowerOf2())) {
> + if (N1C && !N1C->isNullValue() && (N1C->getAPIntValue().isPowerOf2() ||
> + (-N1C->getAPIntValue()).isPowerOf2()))
> {
> // If dividing by powers of two is cheap, then don't perform the following
> // fold.
> if (TLI.isPow2DivCheap())
> return SDValue();
>
> - unsigned lg2 = Divisor->countTrailingZeros();
> + unsigned lg2 = N1C->getAPIntValue().countTrailingZeros();
>
> // Splat the sign bit into the register
> SDValue SGN =
> @@ -2025,7 +2020,7 @@ SDValue DAGCombiner::visitSDIV(SDNode *N
>
> // If we're dividing by a positive value, we're done. Otherwise, we must
> // negate the result.
> - if (Divisor->isNonNegative())
> + if (N1C->getAPIntValue().isNonNegative())
> return SRA;
>
> AddToWorkList(SRA.getNode());
> @@ -2034,7 +2029,7 @@ SDValue DAGCombiner::visitSDIV(SDNode *N
>
> // if integer divide is expensive and we satisfy the requirements, emit an
> // alternate sequence.
> - if ((N1C || N1->getOpcode() == ISD::BUILD_VECTOR) &&
> !TLI.isIntDivCheap()) {
> + if (N1C && !TLI.isIntDivCheap()) {
> SDValue Op = BuildSDIV(N);
> if (Op.getNode()) return Op;
> }
> @@ -2052,8 +2047,8 @@ SDValue DAGCombiner::visitSDIV(SDNode *N
> SDValue DAGCombiner::visitUDIV(SDNode *N) {
> SDValue N0 = N->getOperand(0);
> SDValue N1 = N->getOperand(1);
> - ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0.getNode());
> - ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1.getNode());
> + ConstantSDNode *N0C = isConstOrConstSplat(N0);
> + ConstantSDNode *N1C = isConstOrConstSplat(N1);
> EVT VT = N->getValueType(0);
>
> // fold vector ops
> @@ -2086,7 +2081,7 @@ SDValue DAGCombiner::visitUDIV(SDNode *N
> }
> }
> // fold (udiv x, c) -> alternate
> - if ((N1C || N1->getOpcode() == ISD::BUILD_VECTOR) &&
> !TLI.isIntDivCheap()) {
> + if (N1C && !TLI.isIntDivCheap()) {
> SDValue Op = BuildUDIV(N);
> if (Op.getNode()) return Op;
> }
> @@ -2104,8 +2099,8 @@ SDValue DAGCombiner::visitUDIV(SDNode *N
> SDValue DAGCombiner::visitSREM(SDNode *N) {
> SDValue N0 = N->getOperand(0);
> SDValue N1 = N->getOperand(1);
> - ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0);
> - ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1);
> + ConstantSDNode *N0C = isConstOrConstSplat(N0);
> + ConstantSDNode *N1C = isConstOrConstSplat(N1);
> EVT VT = N->getValueType(0);
>
> // fold (srem c1, c2) -> c1%c2
> @@ -2146,8 +2141,8 @@ SDValue DAGCombiner::visitSREM(SDNode *N
> SDValue DAGCombiner::visitUREM(SDNode *N) {
> SDValue N0 = N->getOperand(0);
> SDValue N1 = N->getOperand(1);
> - ConstantSDNode *N0C = dyn_cast<ConstantSDNode>(N0);
> - ConstantSDNode *N1C = dyn_cast<ConstantSDNode>(N1);
> + ConstantSDNode *N0C = isConstOrConstSplat(N0);
> + ConstantSDNode *N1C = isConstOrConstSplat(N1);
> EVT VT = N->getValueType(0);
>
> // fold (urem c1, c2) -> c1%c2
> @@ -11187,28 +11182,20 @@ SDValue DAGCombiner::SimplifySetCC(EVT V
> /// multiplying by a magic number. See:
> /// <http://the.wall.riscom.net/books/proc/ppc/cwg/code2.html>
> SDValue DAGCombiner::BuildSDIV(SDNode *N) {
> - const APInt *Divisor;
> - if (N->getValueType(0).isVector()) {
> - // Handle splat vectors.
> - BuildVectorSDNode *BV = cast<BuildVectorSDNode>(N->getOperand(1));
> - if (ConstantSDNode *C = BV->getConstantSplatValue())
> - Divisor = &C->getAPIntValue();
> - else
> - return SDValue();
> - } else {
> - Divisor = &cast<ConstantSDNode>(N->getOperand(1))->getAPIntValue();
> - }
> + ConstantSDNode *C = isConstOrConstSplat(N->getOperand(1));
> + if (!C)
> + return SDValue();
>
> // Avoid division by zero.
> - if (!*Divisor)
> + if (!C->getAPIntValue())
> return SDValue();
>
> std::vector<SDNode*> Built;
> - SDValue S = TLI.BuildSDIV(N, *Divisor, DAG, LegalOperations, &Built);
> + SDValue S =
> + TLI.BuildSDIV(N, C->getAPIntValue(), DAG, LegalOperations, &Built);
>
> - for (std::vector<SDNode*>::iterator ii = Built.begin(), ee = Built.end();
> - ii != ee; ++ii)
> - AddToWorkList(*ii);
> + for (SDNode *N : Built)
> + AddToWorkList(N);
> return S;
> }
>
> @@ -11217,28 +11204,20 @@ SDValue DAGCombiner::BuildSDIV(SDNode *N
> /// multiplying by a magic number. See:
> /// <http://the.wall.riscom.net/books/proc/ppc/cwg/code2.html>
> SDValue DAGCombiner::BuildUDIV(SDNode *N) {
> - const APInt *Divisor;
> - if (N->getValueType(0).isVector()) {
> - // Handle splat vectors.
> - BuildVectorSDNode *BV = cast<BuildVectorSDNode>(N->getOperand(1));
> - if (ConstantSDNode *C = BV->getConstantSplatValue())
> - Divisor = &C->getAPIntValue();
> - else
> - return SDValue();
> - } else {
> - Divisor = &cast<ConstantSDNode>(N->getOperand(1))->getAPIntValue();
> - }
> + ConstantSDNode *C = isConstOrConstSplat(N->getOperand(1));
> + if (!C)
> + return SDValue();
>
> // Avoid division by zero.
> - if (!*Divisor)
> + if (!C->getAPIntValue())
> return SDValue();
>
> std::vector<SDNode*> Built;
> - SDValue S = TLI.BuildUDIV(N, *Divisor, DAG, LegalOperations, &Built);
> + SDValue S =
> + TLI.BuildUDIV(N, C->getAPIntValue(), DAG, LegalOperations, &Built);
>
> - for (std::vector<SDNode*>::iterator ii = Built.begin(), ee = Built.end();
> - ii != ee; ++ii)
> - AddToWorkList(*ii);
> + for (SDNode *N : Built)
> + AddToWorkList(N);
> return S;
> }
>
>
> Modified: llvm/trunk/test/CodeGen/X86/vector-idiv.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vector-idiv.ll?rev=207338&r1=207337&r2=207338&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/vector-idiv.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/vector-idiv.ll Sat Apr 26 18:09:49 2014
> @@ -151,3 +151,38 @@ define <8 x i32> @test9(<8 x i32> %a) {
> ; AVX: vpsrad $2
> ; AVX: vpadd
> }
> +
> +define <8 x i32> @test10(<8 x i32> %a) {
> + %rem = urem <8 x i32> %a, <i32 7, i32 7, i32 7, i32 7,i32 7, i32 7, i32
> 7, i32 7>
> + ret <8 x i32> %rem
> +
> +; AVX-LABEL: test10:
> +; AVX: vpermd
> +; AVX: vpmuludq
> +; AVX: vshufps $-35
> +; AVX: vpmuludq
> +; AVX: vshufps $-35
> +; AVX: vpsubd
> +; AVX: vpsrld $1
> +; AVX: vpadd
> +; AVX: vpsrld $2
> +; AVX: vpmulld
> +}
> +
> +define <8 x i32> @test11(<8 x i32> %a) {
> + %rem = srem <8 x i32> %a, <i32 7, i32 7, i32 7, i32 7,i32 7, i32 7, i32
> 7, i32 7>
> + ret <8 x i32> %rem
> +
> +; AVX-LABEL: test11:
> +; AVX: vpermd
> +; AVX: vpmuldq
> +; AVX: vshufps $-35
> +; AVX: vpmuldq
> +; AVX: vshufps $-35
> +; AVX: vpshufd $-40
> +; AVX: vpadd
> +; AVX: vpsrld $31
> +; AVX: vpsrad $2
> +; AVX: vpadd
> +; AVX: vpmulld
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
More information about the llvm-commits
mailing list