This is an archive of the discontinued LLVM Phabricator instance.

[PPC] Change i32 constant in store instruction to i64
ClosedPublic

Authored by Carrot on Nov 9 2017, 3:51 PM.

Details

Summary

The motivated test case is:

struct S {

long l;
char c;
short s;
int i;

};

void foo(struct S *p) {

p->l = 0;
p->c = 0;
p->s = 0;
p->i = 0;

}

It is very common in constructors, llvm generates:

li 4, 0
li 5, 0          <=== this is not necessary, we could use r4 instead.
stb 4, 0(3)
sth 4, 2(3)
stw 4, 4(3)
std 5, 8(3)
blr

It is because 8bit and 16bit types are not legal, they are promoted to 32bit, so all 8b/16b/32b stores reference Constant:i32<0>, constants are shared in LLVM IR, so they are naturally CSEd. 64bit store references Constant:i64<0>, it is of different type, so it is represented by a different constant node, and generate another constant construction instruction.

This patch changes all i32 constant in store instruction to i64 with truncation, to increase the chance that the referenced constant can be shared with other i64 constant.

Diff Detail

Event Timeline

Carrot created this revision.Nov 9 2017, 3:51 PM
nemanjai edited edge metadata.Nov 10 2017, 3:27 AM

Thanks for taking care of this, I think it's quite useful and it should reduce the number of load immediate instructions and registers used. However, do we really need to expose a new API in StoreSDNode? Can we not just create a new truncating store using SelectionDAG::getTruncStore()? That would make this look a lot more like the rest of the DAG combines.

Thanks for taking care of this, I think it's quite useful and it should reduce the number of load immediate instructions and registers used. However, do we really need to expose a new API in StoreSDNode? Can we not just create a new truncating store using SelectionDAG::getTruncStore()? That would make this look a lot more like the rest of the DAG combines.

There are two SelectionDAG::getTruncStore() functions, but neither of them can accept the general (base + offset) addressing mode, so if we use getTruncStore, we still need UpdateNodeOperands to add offset if there is one.

There are two SelectionDAG::getTruncStore() functions, but neither of them can accept the general (base + offset) addressing mode, so if we use getTruncStore, we still need UpdateNodeOperands to add offset if there is one.

Fair enough, it's unfortunate that there isn't an overload that can do the job. And providing another overload is probably even more intrusive than adding a mutator function like this to StoreSDNode. Can you please add a comment explaining that in the code since this looks different from other combines and others may wonder why we didn't just use DAG.getTruncStore()?

LGTM.

include/llvm/CodeGen/SelectionDAGNodes.h
2018

I think this line might be too long.

nemanjai accepted this revision.Nov 14 2017, 3:37 PM

Forgot to select "Accept"...

This revision is now accepted and ready to land.Nov 14 2017, 3:37 PM
Carrot updated this revision to Diff 123063.Nov 15 2017, 11:25 AM
Carrot marked an inline comment as done.

Will check in this version.

Carrot closed this revision.Nov 16 2017, 10:49 AM

Committed as r318436.