This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Improve X div/rem Y fold if single bit element type
ClosedPublic

Authored by xbolva00 on Sep 28 2018, 1:06 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Sep 28 2018, 1:06 PM
spatel added inline comments.Sep 29 2018, 8:40 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3133 ↗(On Diff #167534)

There should be a TODO comment on the zext line because we don't handle that pattern here.

test/CodeGen/X86/combine-urem.ll
388 ↗(On Diff #167534)

This test should use 'urem' not 'srem'.
Please commit the tests with baseline assertions as a preliminary step.

xbolva00 updated this revision to Diff 167624.Sep 29 2018, 2:12 PM
  • Updated and rebase

xbolva00@xbolva00:~/LLVM/llvm/utils$ ./update_llc_test_checks.py /home/xbolva00/LLVM/llvm/test/CodeGen/Mips/llvm-ir/urem.ll
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'
Cannot find a triple. Assume 'x86'

WARNING: Found conflicting asm under the same prefix: 'NOT-R6'! WARNING: Found conflicting asm under the same prefix: 'NOT-R6'! Cannot find a triple. Assume 'x86' Cannot find a triple. Assume 'x86' Cannot find a triple. Assume 'x86'

and then test crashes :(
/home/xbolva00/LLVM/llvm/test/CodeGen/Mips/llvm-ir/srem.ll:439:16: error: NOT-R6-NEXT: expected string not found in input
; NOT-R6-NEXT: daddiu $sp, $sp, -16

xbolva00@xbolva00:~/LLVM/llvm/utils$ ./update_llc_test_checks.py /home/xbolva00/LLVM/llvm/test/CodeGen/Mips/llvm-ir/urem.ll
Cannot find a triple. Assume 'x86'

You need to replace -march with -mtriple in RUN statements before running the update_llc_test_checks.py. For example, -march=mips becomes -mtriple=mips.

Thanks, one more problem:

WARNING: Skipping non-opt RUN line: llc < %s -mtriple=mips -mcpu=mips32r3 -mattr=+micromips -relocation-model=pic | FileCheck %s -check-prefixes=ALL,MMR3,MM32 WARNING: Skipping non-opt RUN line: llc < %s -mtriple=mips -mcpu=mips32r6 -mattr=+micromips -relocation-model=pic | FileCheck %s -check-prefixes=ALL,MMR6,MM32 ...

Thanks, one more problem:

WARNING: Skipping non-opt RUN line: llc < %s -mtriple=mips -mcpu=mips32r3 -mattr=+micromips -relocation-model=pic | FileCheck %s -check-prefixes=ALL,MMR3,MM32 WARNING: Skipping non-opt RUN line: llc < %s -mtriple=mips -mcpu=mips32r6 -mattr=+micromips -relocation-model=pic | FileCheck %s -check-prefixes=ALL,MMR6,MM32 ...

Are you using update_llc_test_checks.py ? The "non-opt" remark makes me think you've used update_test_checks.py

xbolva00 added a comment.EditedSep 30 2018, 2:20 AM

Uh, I messed up. But still some warnings:
LLVM/llvm/test/CodeGen/Mips/llvm-ir/urem.ll

WARNING: Found conflicting asm under the same prefix: 'NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'R2-R6'!
WARNING: Found conflicting asm under the same prefix: 'NOT-R2-R6'!
WARNING: Found conflicting asm under the same prefix: 'NOT-R6'!
WARNING: Found conflicting asm under the same prefix: 'R2-R6'!
WARNING: Found conflicting asm under the same prefix: 'MM32'!

LLVM/llvm/test/CodeGen/Mips/llvm-ir/udiv.ll

WARNING: Found conflicting asm under the same prefix: 'GP32'!
WARNING: Found conflicting asm under the same prefix: 'MM32'!
spatel added inline comments.Sep 30 2018, 7:03 AM
test/CodeGen/Mips/llvm-ir/urem.ll
40–43 ↗(On Diff #167624)

All of this non-code output suggests that:

  1. We need to refine the script for MIPS asm.
  2. You might want to just hand-edit the existing test(s) where your patch makes a difference.

If you do want to auto-gen the checks for this file, that should be done before this patch in a NFC commit.

I will fix the MIPS test cases soon.

I will fix the MIPS test cases soon.

Thanks!

xbolva00 updated this revision to Diff 167956.Oct 2 2018, 9:07 AM
  • Updated tests

I hope rL343485 helps.

Yes, amazing work. Thanks. Now the patch passes the test suite.

RKSimon added inline comments.Oct 2 2018, 9:41 AM
test/CodeGen/X86/pr38539.ll
5 ↗(On Diff #167956)

These reduced tests need to be regenerated from the source in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8882

xbolva00 added inline comments.Oct 2 2018, 10:29 AM
test/CodeGen/X86/pr38539.ll
5 ↗(On Diff #167956)
xbolva00 added a comment.EditedOct 18 2018, 3:06 PM

I reverted D50656 fix for pr38539 and pr38539.ll passed fine. So pr38539.ll tests nothing in the current trunk and it should not block this patch.

Failing Tests (1):

  LLVM :: CodeGen/X86/flags-copy-lowering.mir

Expected Passes    : 11546
Expected Failures  : 53
Unsupported Tests  : 229
Unexpected Failures: 1
spatel accepted this revision.Oct 29 2018, 4:20 PM

LGTM - the MIR test should be adequate coverage for rL339945.

This revision is now accepted and ready to land.Oct 29 2018, 4:20 PM
This revision was automatically updated to reflect the committed changes.