This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Optimize (add (shl x, c0), c1)
AbandonedPublic

Authored by benshi001 on Oct 4 2021, 9:59 PM.

Details

Summary
Transform (add (shl x, c0), c1) ->
          (add (shl (add x, c1>>c0), c0), c1-(c1>>c0<<c0)),
if c1>>c0 and c1-(c1>>c0<<c0) are simm12, while c1 is not.

Or transform (add (shl x, c0), c1) ->
             (shl (add x, c1>>c0), c0),
if c1-(c1>>c0<<c0) is zero, and c1>>c0 is simm12 while c1 is not.

Diff Detail

Event Timeline

benshi001 created this revision.Oct 4 2021, 9:59 PM
benshi001 requested review of this revision.Oct 4 2021, 9:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 9:59 PM
benshi001 edited the summary of this revision. (Show Details)Oct 5 2021, 7:02 AM
benshi001 set the repository for this revision to rG LLVM Github Monorepo.
craig.topper added inline comments.Oct 13 2021, 10:13 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6477

I think you can drop this if, getNode will see the constant is 0 and not produce an ADD node.

6483–6484

Just drop this comment. It's already above transformAddImmMulImm.

6484

This comment is already above transformAddImmShlImm, we don't need to repeat it.

benshi001 updated this revision to Diff 379574.Oct 13 2021, 6:20 PM
benshi001 marked 3 inline comments as done.
benshi001 added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6454

This comment also repeated before the definition of transformAddShlImm, and should also be dropped.

benshi001 updated this revision to Diff 379586.Oct 13 2021, 7:40 PM
craig.topper added inline comments.Oct 13 2021, 10:03 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6468

I think you need to abort if C0 is greater than VT.getSizeInBits() - 1.

benshi001 marked an inline comment as done.
benshi001 updated this revision to Diff 380269.Oct 17 2021, 7:41 PM

ping ... Any concerns about this patch ?

asb added a comment.Nov 2 2021, 7:55 AM

I'm getting a warning when building this (with clang 12.0.1):

/home/asb/llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6490:20: warning: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Wsign-compare]
  if (C0 < 1 || C0 > VT.getSizeInBits() - 1 || isInt<12>(C1) ||
benshi001 updated this revision to Diff 384121.Nov 2 2021, 8:34 AM

I'm getting a warning when building this (with clang 12.0.1):

/home/asb/llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6490:20: warning: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Wsign-compare]
  if (C0 < 1 || C0 > VT.getSizeInBits() - 1 || isInt<12>(C1) ||

I have fixed in the newest patch.

asb added a comment.Nov 5 2021, 7:13 AM

I'm seeing some code size regressions on this patch - specifically, cases where a reg-reg add is replaced with an addi -256 (which isn't compressible). e.g. 20050121-1.c from the GCC torture suite (-O1, rv32imafdc).

benshi001 abandoned this revision.Nov 7 2021, 7:17 PM

I'm seeing some code size regressions on this patch - specifically, cases where a reg-reg add is replaced with an addi -256 (which isn't compressible). e.g. 20050121-1.c from the GCC torture suite (-O1, rv32imafdc).

Thanks for your patient and careful test, I will made a new patch for SHL related optimization after a careful check.