[SeparateConstOffsetFromGEPPass] Added optional modification strategy

[eklepilkina]

Summary

This modification strategy tries to understand which GEP instrucions is profitable to modify for register pressure decreasing.

[anton-afanasyev]

Please rebase against precommited tests.

[eklepilkina]

Rebased against precommited tests

[reames]

At least for me, I need some context to be able to review this. What is the case which this improves in terms of codegen? And how common are such patterns? Keep in mind, I'm not terribly familiar with the pass here, so this may be pretty basic explanation. Do you have a bug with examples or analyze that lead to this change?

[eklepilkina]

Sorry, I had to provide the context at the beginning.

Now clang for RISC-V doesn't use offset addressing in generated assembly. Example from Dhrystone

 addiw   a0, s1, 5
 slli    a1, a0, 0x2
 add     a2, s4, a1
 sw      s2, 0(a2)
 addiw   a3, s1, 6
 slli    a3, a3, 0x2
 add     a3, a3, s4
 sw      s2, 0(a3)
 addiw   a3, s1, 35
 slli    a3, a3, 0x2
add     a3, a3, s4
sw      a0, 0(a3)

It's inefficient because we can use offsets.
Adding this pass allows to generate the next code

    addiw   a4, a2, 5
    slli    a5, a2, 2
    add a0, a0, a5
    sw  a3, 20(a0)
    sw  a3, 24(a0)
    sw  a4, 140(a0)
...

SeparateConstOffsetFromGEPPass is used to solve this problem in targets with limited addressing modes.
The changes inside pass was made because modification of all GEPs isn't profitable, seems that we need at least 2 GEPs and one value that was used for index that can be removed after modification. Otherwise we don't decrease register pressure.

[craig.topper]

This should be two patches, one changing the pass and one enabling for RISC-V.

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

171 ↗

(On Diff #436758)

Can we add a command line option to control this like AArch64 and PowerPC have?

llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll

2

This test doesn't exist in the repo. Where is the patch that adds it?

[eklepilkina]

llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll

2

I was told in the first comment to rebase on precommited tests. These tests are added as precommited in separate commit. Should I commit them?

[craig.topper]

llvm/test/Transforms/SeparateConstOffsetFromGEP/RISCV/split-gep.ll

2

Why is the script NOTE: Assertions have been autogenerated by utils/update_test_checks.py not in the pre-committed version?

[eklepilkina]

This should be two patches, one changing the pass and one enabling for RISC-V.

As far as I want to turn pass with enabled strategy, should I wait approve and merge of the accepting strategy and only after this create the second review? Or create series of patches as mentionedin documentation https://llvm.org/docs/Phabricator.html#creating-a-patch-series?

[craig.topper]

llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp

365

I think there you should be a std::move on PreviousIndices

368

rhs -> RHS

954–958

PossibleBase.size() == 0 -> PossibleBases.empty()

[craig.topper]

In D127727#3582087, @eklepilkina wrote:

This should be two patches, one changing the pass and one enabling for RISC-V.

As far as I want to turn pass with enabled strategy, should I wait approve and merge of the accepting strategy and only after this create the second review? Or create series of patches as mentionedin documentation https://llvm.org/docs/Phabricator.html#creating-a-patch-series?

You should create a series of patches.

[eklepilkina]

Separate part with pass modification

[eklepilkina]

• [SeparateConstOffsetFromGEP] Fix comparator for map with GEP bases

[eklepilkina]

Refactoring

[eklepilkina]

Fix review

[eklepilkina]

Ping

[eklepilkina]

• [SeparateConstOffsetFromGEP] Fix ignoring condition

[eklepilkina]

Gentle ping