This is an archive of the discontinued LLVM Phabricator instance.

InstCombine: propagate deref via new addDereferenceableAttr
ClosedPublic

Authored by artagnon on Feb 9 2015, 9:54 AM.

Details

Summary

The "dereferenceable" attribute cannot be added via .addAttribute(),
since it also expects a size in bytes. AttrBuilder#addAttribute or
AttributeSet#addAttribute is wrapped by classes Function, InvokeInst,
and CallInst. Add corresponding wrappers to
AttrBuilder#addDereferenceableAttr.

Having done this, propagate the dereferenceable attribute via
gc.relocate, adding a test to exercise it. Note that -datalayout is
required during execution over and above -instcombine, because
InstCombine only optionally requires DataLayoutPass.

Diff Detail

Repository
rL LLVM

Event Timeline

artagnon updated this revision to Diff 19595.Feb 9 2015, 9:54 AM
artagnon retitled this revision from to InstCombine: propagate deref via new addDereferenceableAttr.
artagnon updated this object.
artagnon edited the test plan for this revision. (Show Details)
artagnon added reviewers: hfinkel, echristo, sanjoy, reames.
artagnon added a subscriber: Unknown Object (MLST).
reames edited edge metadata.Feb 9 2015, 1:46 PM

General direction seems fine. I can't comment on the attribute set stuff (I've never taken the time to understand it), maybe Hal can?

lib/Transforms/InstCombine/InstCombineCalls.cpp
1139 ↗(On Diff #19595)

This cast is not safe. Dereferencabble pointers can come from other sources (e.g. calls) as well.

artagnon updated this revision to Diff 19613.Feb 9 2015, 2:06 PM
artagnon edited edge metadata.

Don't assume that the derived pointer can always be cast to an
Argument; dyn_cast and check.

reames added a comment.Feb 9 2015, 2:26 PM

GC parts LGTM; explicitly not addressing attribute changes

p.s. isDerefPtr is on Value, the cast is entirely unnecessary.

p.s. isDerefPtr is on Value, the cast is entirely unnecessary.

Ah, but ->getDereferenceableBytes() is only defined for an Argument; I'm not sure why it is that way though.

reames added a comment.Feb 9 2015, 3:32 PM

p.s. isDerefPtr is on Value, the cast is entirely unnecessary.

Ah, but ->getDereferenceableBytes() is only defined for an Argument; I'm not sure why it is that way though.

Ok, I see why you need the cast. Another idea for later cleanup. :)

hfinkel accepted this revision.Feb 10 2015, 12:02 PM
hfinkel edited edge metadata.

The changes adding addDereferenceableAttr functions LGTM, so I think you're good to go.

This revision is now accepted and ready to land.Feb 10 2015, 12:02 PM
This revision was automatically updated to reflect the committed changes.