Page MenuHomePhabricator

Make WholeProgramDevirt understand ConstStruct vtables.
ClosedPublic

Authored by LemonBoy on Nov 12 2016, 7:04 AM.

Details

Summary

I've added another test to the non-array-vtable file, I hope that's the correct place.

Diff Detail

Event Timeline

LemonBoy updated this revision to Diff 77723.Nov 12 2016, 7:04 AM
LemonBoy retitled this revision from to Make WholeProgramDevirt understand ConstStruct vtables..
LemonBoy updated this object.
pcc added inline comments.Nov 27 2016, 9:28 PM
lib/Transforms/IPO/WholeProgramDevirt.cpp
425–430

All this code could be factored out into a separate function.

432

I think you also want to check that SL->getElementOffset(Op) == GlobalSlotOffset.

Please add negative tests for that case and the out-of-bounds case you have above.

test/Transforms/WholeProgramDevirt/non-aggregate-vtable.ll
30

Please put your test in a separate file and rename this to something like non-aggregate-vtable.ll.

LemonBoy updated this revision to Diff 79457.Nov 28 2016, 2:08 PM
LemonBoy marked 3 inline comments as done.
LemonBoy added inline comments.
lib/Transforms/IPO/WholeProgramDevirt.cpp
432

I haven't been able to come up with a way to access the table mid-element via a GEP but have added the check anyway.

pcc added inline comments.Nov 28 2016, 2:13 PM
lib/Transforms/IPO/WholeProgramDevirt.cpp
432

See e.g. the unaligned test in bad-read-from-vtable.ll.

LemonBoy updated this revision to Diff 79460.Nov 28 2016, 2:19 PM
LemonBoy marked an inline comment as done.
pcc added inline comments.Nov 28 2016, 2:33 PM
lib/Transforms/IPO/WholeProgramDevirt.cpp
386

Could be simpler to return a const Constant * from this function.

426–428

These two could be folded into the only use (after applying my suggestion above).

test/Transforms/WholeProgramDevirt/non-aggregate-vtable.ll
1

struct-vtable.ll would be a better name for this test.

LemonBoy updated this revision to Diff 80803.Dec 8 2016, 12:02 PM
LemonBoy marked an inline comment as done.Dec 8 2016, 12:04 PM
LemonBoy added inline comments.
lib/Transforms/IPO/WholeProgramDevirt.cpp
386

I suppose you meant const Value * since that's what getOperand returns.

pcc accepted this revision.Dec 8 2016, 12:30 PM
pcc edited edge metadata.

LGTM with nit

lib/Transforms/IPO/WholeProgramDevirt.cpp
386

getOperand on a constant will only ever return a constant; you could cast it below.

This revision is now accepted and ready to land.Dec 8 2016, 12:30 PM
pcc closed this revision.Dec 8 2016, 4:46 PM

It doesn't look like you have commit access, so I committed in r289162. I had to remove some of the const qualification to make it compile.