Page MenuHomePhabricator

-Oz: Reuse constants in registers instead of canonicalizing operations that use a different constant
Needs RevisionPublic

Authored by ramred01 on May 2 2019, 5:06 AM.

Details

Summary

This patch fixes an issue with -Oz and -Os compilation where it is profitable to reuse constants that are already in registers rather than canonicalizing an operation that changes the constant used, thereby having to materialize the constant again.

e.g:
int test(int* x)
{

if (x > (int)655360000L)
   return *x - (int)655360000L;
return x;

}

In the above code, although the constant used in the compare is reused in the subtraction, LLVM canonicalizes the subtraction to an addition with a negative constant. That means that the constant that was materialized in a register for the compare cannot be reused for the subsequent operation and another constant (which is the negative of the first one) is materialized.

We solve this by tracking the constants that are used in a function and wherever we see a new constant, we check is it is the negative of an earlier used constant or just one off from an earlier used constant. If so, then we reverse the canonicalization for that instruction so as to use the same constant as was used before.

Diff Detail

Event Timeline

ramred01 created this revision.May 2 2019, 5:06 AM
ostannard requested changes to this revision.May 2 2019, 9:41 AM
ostannard edited reviewers, added: ostannard; removed: olista01.
ostannard added a subscriber: ostannard.
ostannard added inline comments.
include/llvm/CodeGen/TargetInstrInfo.h
580

This name shouldn't mention AArch64 if it can be implemented for other targets, and shouldn't be here if it is AArch64 specific. It should also mention _what_ it does, not just "change".

lib/CodeGen/MachineCSE.cpp
62

What if the value doesn't fit into an int (on the host system)?

509

AC6?

516

This name tells me nothing about what the function does.

523

I don't think we can guarantee that operands 0 and 1 are used consistently across all targets.

531

If both positive and negative versions of the same immediate exist in the function, isn't this going to swap them both, and we'll still have both constants?

Why do we want to allow an offset of one? I don't see any code below which adjusts for this, so isn't this going to change the constant?

559

Why? Also, this check will only detect global isel compare instructions, not target-specific ones.

593

Duplicated call

595

Again, this is making assumptions about operands which might not be true for all targets.

599

This is going to replace every use of the register, not just the one we have checked is safe.

I don't see any checks to make sure the add/sub instruction is dominated by the immediate move which we are re-using.

623

Why is this done here? It should be either properly integrated into the CSE algorithm, or made a separate pass.

This revision now requires changes to proceed.May 2 2019, 9:41 AM