Page MenuHomePhabricator

[InferAttrs] Improve DSE for libcalls (partial fix for PR47644)
Needs ReviewPublic

Authored by xbolva00 on Sep 25 2020, 12:06 PM.

Details

Summary

Optimize `
void f(const char *s) {

char a[256];
__builtin_strcpy(a, s);  // dead store

}

to just empty function.

Diff Detail

Unit TestsFailed

TimeTest
25,160 mslinux > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

xbolva00 created this revision.Sep 25 2020, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 12:06 PM
xbolva00 requested review of this revision.Sep 25 2020, 12:06 PM

Interestingly, as we can see, something is broken with NPM (hopefully not a blocker for this patch)

I think it would be good to update the commit message to just state that this adds no capture and write only to the first arg of strcpy & co. It also unifies strcpy & stpcpy handling. might be good to do separately first.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
11

Can we just add those tests to llvm/test/Transforms/DeadStoreElimination/MSSA/libcalls.ll?

fhahn added inline comments.Sep 25 2020, 12:53 PM
llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
19

I think the NPM would require -aa-pipeline=basic-aa. But given that the interaction between -infer-attrs/-dse is very small, I think it would be best to move the checks to the existing lib calls.ll

I think it would be good to update the commit message to just state that this adds no capture and write only to the first arg of strcpy & co. It also unifies strcpy & stpcpy handling. might be good to do separately first.

Ok.

So first step: https://reviews.llvm.org/D88335

Needs a rebase.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
13

I'm unsure how this is optimized because I don't see argmemonly on the strcpy. Did I miss it? If not, what is the justification here?

Ok, I should be ready to make progress here again.

@fhahn, with manually added writeonly, this optimization no longer works for me somehow.

https://godbolt.org/z/jErj3f

Any recent change in MSSA DSE which could cause this behaviour?

xbolva00 added a comment.EditedSun, Oct 18, 4:33 AM

Test cases work for me with clang/opt 11 with mssa dse

https://godbolt.org/z/648nMq

https://godbolt.org/z/qPaMME

fhahn added a comment.Sun, Oct 18, 7:51 AM

Test cases work for me with clang/opt 11 with mssa dse

https://godbolt.org/z/648nMq

https://godbolt.org/z/qPaMME

I think there was a change how lifetime.end is handled in MemorySSA. Adjusted DSE in f5cf7f544b7abe8488f76945537044f700b5548a.

llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
13

I think this is due to an inconsistency with the attributes. The implicit assumption is that MemoryLocation::getForArgument currently only returns a proper location for argmemonly I think. We should mark them as argmemonly and then probably have an assert in getLocationFor that is is only used for such functions.

xbolva00 added inline comments.Sun, Oct 18, 8:00 AM
llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
13

We should mark them as argmemonly

This part is done (I committed it yesterday)

Thanks for fast fix!

fhahn added inline comments.Sun, Oct 18, 10:21 AM
llvm/test/Transforms/PhaseOrdering/dse-libcalls.ll
13

nice, so I guess it should be safe to add the assert now.