Page MenuHomePhabricator

[ConstHoist] Do not rebase single (or few) dependent constant
ClosedPublic

Authored by zzheng on Sep 18 2018, 11:48 AM.

Details

Summary

If an instance (InsertionPoint or IP) of Base constant A has only one or few rebased constants depending on it, do NOT rebase. One extra ADD instruction is required to materialize each rebased constant, assuming A and the rebased have the same materialization cost.

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng created this revision.Sep 18 2018, 11:48 AM

This does seem, from the tests I ran, to reduce codesize on average. Especially on Thumb1.

lib/Transforms/Scalar/ConstantHoisting.cpp
873 ↗(On Diff #166012)

Can you change this around so we don't speculatively make the Base? That seems to be the way of doing things in LLVM.

test/CodeGen/Thumb/consthoist-single-dependent.ll
1 ↗(On Diff #166012)

Would these tests make more sense as opt tests? That way they can just test this specific pass, not the entire backend.

123 ↗(On Diff #166012)

Clean up tests please.

dmgreen added inline comments.Sep 21 2018, 2:06 AM
test/CodeGen/Thumb/consthoist-single-dependent.ll
1 ↗(On Diff #166012)

Perhaps as-well-as, not instead-of? If you still want to test this behaviour too.

zzheng updated this revision to Diff 166580.Sep 21 2018, 5:16 PM
zzheng retitled this revision from [ConstHoist]Do NOT rebase single-dependent of base constant to [ConstHoist] Do NOT rebase single (or few) dependent of base insertion point.

Cleaned up test case.
Added opt tests.
Opt tests was put in the same test file to avoid duplication and maintenance burden.

zzheng marked 3 inline comments as done.Sep 21 2018, 5:25 PM
zzheng added inline comments.
lib/Transforms/Scalar/ConstantHoisting.cpp
873 ↗(On Diff #166012)

Here Base is not the base constant, but an instance (insertion point) of it. A base constant can have multiple insertion points. The existing algorithm first finds the insertion points, then looks for rebased uses dominated by each insertion point.

If I understand your comments correctly, it's desirable to not to have Base at the first place so no need to erase it later. But we need the number of rebased uses to determine if we'll rewrite such uses. To get this number we need Base.

I can remove the statement:

Base->eraseFromParent();

Since Base has no use, it'll be trivially removed by DCE.

This does seem, from the tests I ran, to reduce codesize on average. Especially on Thumb1.

Yes, we're trying to reduce code size on Thumb1 for an internal workload.

dmgreen added inline comments.Sep 23 2018, 12:54 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
873 ↗(On Diff #166012)

(I think) because we are inserting Base at IP, Base->getParent will just be IP->getParent, and if we use that we can move the code to create Base (and the debug message after) to after this check.

Unless findMatInsertPt is doing something I don't expect and requiring that Base has been created? Make sure that works, and all the tests are passing.

test/CodeGen/Thumb/consthoist-single-dependent.ll
14 ↗(On Diff #166580)

Can you add the bb these are expected to be in, like in "carla" below. I think thats good at makes the intent a lot clearer.

zzheng updated this revision to Diff 166766.Sep 24 2018, 3:26 PM
zzheng retitled this revision from [ConstHoist] Do NOT rebase single (or few) dependent of base insertion point to [ConstHoist] Do not rebase single (or few) dependent constant.
zzheng edited the summary of this revision. (Show Details)

Moved collecting dependent rebased and checking size of ToBeRebased prior to emitting bitcast of base constants
Added bb labels to check in the test case.

zzheng marked 4 inline comments as done.Sep 24 2018, 3:28 PM
zzheng added inline comments.
lib/Transforms/Scalar/ConstantHoisting.cpp
873 ↗(On Diff #166012)

Ah, didn't see this. Thanks.

zzheng marked 2 inline comments as done.Sep 24 2018, 3:28 PM
dmgreen accepted this revision.Sep 25 2018, 5:52 AM

LGTM, with one minor Nit.

test/CodeGen/Thumb/consthoist-single-dependent.ll
60 ↗(On Diff #166766)

You can remove this phab reference, or point to the test added there.

This revision is now accepted and ready to land.Sep 25 2018, 5:52 AM
This revision was automatically updated to reflect the committed changes.
zzheng marked an inline comment as done.