Page MenuHomePhabricator

[RISCV] Optimization for using compressed beqz and bnez PR#56391
ClosedPublic

Authored by ita-sc on Aug 29 2022, 2:11 AM.

Details

Summary

Optimization for using compressed beqz and bnez

If there is pattern

br_cc val1 constval eq/neq place
select_cc val1 constval eq/neq trueval falseval

and constval does not fit in compressed imm format(6 bit), but fit in
imm format(12 bit), we can replace by non compress sub and compress
c.beqz/c.bneqz:

addi val val -constval
c.beqz val place

Diff Detail

Event Timeline

ita-sc created this revision.Aug 29 2022, 2:11 AM
ita-sc requested review of this revision.Aug 29 2022, 2:11 AM
ita-sc retitled this revision from Pre-commit tests for PR#56391 to Optimization for using compressed beqz and bnez PR#56391.Aug 29 2022, 2:19 AM
craig.topper retitled this revision from Optimization for using compressed beqz and bnez PR#56391 to [RISCV] Optimization for using compressed beqz and bnez PR#56391.Aug 29 2022, 2:43 PM
craig.topper added inline comments.Aug 29 2022, 3:05 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1334

Please put the branch related code in the branch section of this file.

1341

When wouldn't we have a valid MF?

1346

Blank line before this comment.

ita-sc updated this revision to Diff 456591.Aug 30 2022, 3:09 AM

put branch related code to branch part
adding spaces

ita-sc added inline comments.Aug 30 2022, 3:44 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1334

Done

1341

I guess It happens when we run LLVM IR pass that needs SubTargetImpl.
For example, pass Expand Atomic instructions tries to get SubTargetImpl.
It calls createRISCVInstructionSelector during initialization. In it there is an initialization of predicates for globalISel (see GET_GLOBALISEL_PREDICATES_INIT).
Internally it calls computeAvailableModuleFeatures, where all predicates initilized (usially they use subtarget, that is already initialized), but MF is uninitialized there so there will be error.

1346

Done

liaolucy added inline comments.
llvm/test/CodeGen/RISCV/compress-opt-branch.ll
6

The testcase should be updated with this script, https://github.com/llvm/llvm-project/blob/main/llvm/utils/update_llc_test_checks.py. ignore if already used

ita-sc added inline comments.Sep 5 2022, 6:41 AM
llvm/test/CodeGen/RISCV/compress-opt-branch.ll
6

Hi
Could you please add some information about it?
I tried to do it, but looks like this script doesn't support pattern:
llc | llvm-objdump | FileCheck
as expects llvm-objdump to be llc...

I tried to separate it, but pattern:
llc | llvm-objdump > file
cat file | FileCheck
failed too with message: "Skipping non-FileChecked RUN line".
But I need to update result with llvm-objdump to check for compressed instructions.

liaolucy added inline comments.Sep 5 2022, 6:58 PM
llvm/test/CodeGen/RISCV/compress-opt-branch.ll
6

llvm-project/llvm/test/CodeGen/RISCV/imm.ll as an example:
Command: /llvm-project/llvm/utils/update_llc_test_checks.py --llc-binary=/llvm-project/build/bin/llc llvm-project/llvm/test/CodeGen/RISCV/imm.ll
If the relevant code is modified, the test case will be automatically updated.

But I see that the existing testcases of the c extension(compress-float.ll andcompress.ll) are not updated with the script, so I'm not sure if the testcase of c extension really need to be updated with the script now.

Sorry, please ignore this comment.

A gentle ping.

ita-sc updated this revision to Diff 463234.Sep 27 2022, 7:41 AM

update patches

craig.topper added inline comments.Sep 27 2022, 12:47 PM
llvm/test/CodeGen/RISCV/compress-opt-branch.ll
36

COM isn't a general comment mechanism. It's supposed to be used to temporarily replace a CHECK prefix. Use a regular IR comment.

ita-sc updated this revision to Diff 464635.Oct 3 2022, 2:16 AM

Update review

Have you measured the effect of this on any workloads?

ita-sc added a comment.Oct 4 2022, 3:46 AM

Have you measured the effect of this on any workloads?

Checked on Coremark with/without -Os. Results almost the same. With compressed instructions it is ~1% slower, but size is slightly smaller( this pattern is rare, about 3-4 places where this pattern appears). That is why I added this pattern only when max optimization for size turned on.

craig.topper accepted this revision.Oct 4 2022, 2:07 PM

I guess this is ok. I'm little unsure if this makes sense if the constant has multiple uses, but I also don't think we can reliably identify that in isel.

LGTM

This revision is now accepted and ready to land.Oct 4 2022, 2:07 PM
ita-sc added a comment.Oct 5 2022, 3:01 AM

Thanks @craig.topper . I don’t have commit access, can you land this patch for me? Please use “Ivan Tetyushkin ivan.tetyushkin@syntacore.com” to commit the change.

This revision was landed with ongoing or failed builds.Oct 6 2022, 9:33 AM
This revision was automatically updated to reflect the committed changes.