This is an archive of the discontinued LLVM Phabricator instance.

[Inliner] don't assume that a Constant alloca size is a ConstantInt (PR27277)
ClosedPublic

Authored by spatel on May 9 2016, 12:26 PM.

Diff Detail

Event Timeline

spatel updated this revision to Diff 56608.May 9 2016, 12:26 PM
spatel retitled this revision from to [Inliner] don't assume that a Constant alloca size is a ConstantInt (PR27277).
spatel updated this object.
spatel added reviewers: davidxl, echristo, mehdi_amini.
spatel added a subscriber: llvm-commits.
mehdi_amini added inline comments.May 9 2016, 12:54 PM
lib/Analysis/InlineCost.cpp
335

We're losing the assertion, that could be seen as somehow not totally useless: if the only special case is undef, maybe it could be spelled out explicitly?
(I'm not having a strong opinion either way)

339

Indeed...

My impression is that this is the kind of things that can only be reliably tested using a c++ unitest. I doubt that InlineCost has much unittest coverage though.

spatel added inline comments.May 9 2016, 1:36 PM
lib/Analysis/InlineCost.cpp
335

I've never looked at alloca or this file before, but that seems reasonable (it can't be FP or vector). Let me update the patch.

spatel updated this revision to Diff 56621.May 9 2016, 1:38 PM

Patch updated:
Explicitly check for an undef and keep the assert.

mehdi_amini accepted this revision.May 9 2016, 1:42 PM
mehdi_amini edited edge metadata.

LGTM.

Do you plan to act on the FIXME?

This revision is now accepted and ready to land.May 9 2016, 1:42 PM
majnemer added inline comments.
lib/Analysis/InlineCost.cpp
335

This is wrong. This could will crash if it is a ConstantExpr.

majnemer requested changes to this revision.May 9 2016, 1:45 PM
majnemer added a reviewer: majnemer.
This revision now requires changes to proceed.May 9 2016, 1:45 PM
spatel added a comment.May 9 2016, 1:51 PM
In D20077#425070, @joker.eph wrote:

LGTM.

Thanks!

Do you plan to act on the FIXME?

I would if I knew where the test should go. If you want to handle that, I'll watch and learn. :)

davidxl accepted this revision.May 9 2016, 1:51 PM
davidxl edited edge metadata.

lgtm

davidxl added a subscriber: davidxl.May 9 2016, 1:52 PM

Obviously David M caught a problem ..

spatel updated this revision to Diff 56630.May 9 2016, 2:10 PM
spatel edited edge metadata.

Patch updated:
Back to the original version - if it's not a ConstantInt, we ignore it.

There should be a test-case for the problem David mentioned.

spatel updated this revision to Diff 56633.May 9 2016, 2:24 PM
spatel edited edge metadata.

Patch updated:
Add a ConstExpr test case.

I'd prefer auto * in the if statement

spatel updated this revision to Diff 56636.May 9 2016, 2:39 PM

Patch updated:
Use 'auto' with dyn_cast_or_null.
Also, keep the original 'Size' var because the 80-col forced formatting for the alternative is ugly.

majnemer accepted this revision.May 9 2016, 2:54 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 9 2016, 2:54 PM
This revision was automatically updated to reflect the committed changes.