This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Use IRPosition consistently
ClosedPublic

Authored by jdoerfert on Aug 8 2019, 2:43 PM.

Details

Summary

The next attempt to clean up the Attributor interface before we grow it
further.

Before, we used a combination of two values (associated + anchor) and an
argument number (or -1) to determine a location. This was very fragile.
The new system uses exclusively IR positions and we restrict the
generation of IR positions to special constructor methods that verify
internal constraints we have. This will catch misuse early.

The auto-conversion, e.g., in getAAFor, is now performed through the
SubsumingPositionIterator. This iterator takes an IR position and allows
to visit all IR positions that "subsume" the given one, e.g., function
attributes "subsume" argument attributes of that function. For a
detailed breakdown see the class comment of SubsumingPositionIterator.

This patch also introduces the IRPosition::getAttrs() to extract IR
attributes at a certain position. The method knows how to look up in
different positions that are equivalent, e.g., the argument position for
call site arguments. We also introduce three new positions kinds such
that we have all IR positions where attributes can be placed and one for
"floating" values.

Event Timeline

jdoerfert created this revision.Aug 8 2019, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 2:43 PM

Generally looks good to me. Some initial comments inlined. Since it is a big patch I'd also wait for Hideto to read it. (Maybe I'll reread it as well.)

llvm/include/llvm/Transforms/IPO/Attributor.h
158

I don't see the need for it.

255

You mean if llvm_unreachable makes sense? To me it does. If I'm not wrong this also covers CallSites and I don't see anything else that should be covered.

286

Maybe put these 2 in default with unreachable?

uenoku added a comment.Aug 9 2019, 9:08 AM

LGTM

llvm/include/llvm/Transforms/IPO/Attributor.h
158

What are the advantages to record the scope?

jdoerfert updated this revision to Diff 214722.Aug 12 2019, 3:28 PM
jdoerfert marked 3 inline comments as done.

More IRPosition uses in interfaces, cleanup, adjustment to follow up patches

uenoku added inline comments.Aug 12 2019, 9:05 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
213

What does this TODO mean?

jdoerfert marked an inline comment as done.

Add missing documentation

llvm/include/llvm/Transforms/IPO/Attributor.h
158

We could then tie constants to functions, but again, I was also not sure why/if we want that.

213

It is a reminder to myself to describe the method ;). Done.

255

Yes, unreachable or nullptr was the question. And again, it is about constants (globals, constant integers, ...). It turns out, this is useful in the future so I use nullptr here.

286

I would prefer to make it explicit.

sstefan1 accepted this revision.Aug 13 2019, 3:25 PM

LGTM.

llvm/include/llvm/Transforms/IPO/Attributor.h
158

Ok, so we keep that for now (as it can't do no harm) and see how things go?

This revision is now accepted and ready to land.Aug 13 2019, 3:25 PM
This revision was automatically updated to reflect the committed changes.