Page MenuHomePhabricator

[FunctionAttrs] Conservatively handle operand bundles.
AbandonedPublic

Authored by sanjoy on Sep 22 2015, 6:35 PM.

Details

Summary

This teaches the FunctionAttrs pass to handle operand bundles
conservatively. All calls / invokes with operand bundles are
currently considered to be reading from and writing to the entire
heap.

I've also added a test case PruneEH to show that a call with operand
bundles inherits its callee's behavior with respect to unwinding -- if
the callee is nounwind, the call is nounwind too.

Depends on D12457.

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 35450.Sep 22 2015, 6:35 PM
sanjoy retitled this revision from to [FunctionAttrs] Conservatively handle operand bundles..
sanjoy updated this object.
sanjoy added reviewers: majnemer, chandlerc, nlewycky, reames.
sanjoy added a subscriber: llvm-commits.
sanjoy updated this revision to Diff 35453.Sep 22 2015, 7:29 PM
  • rebase over recent to FunctionAttrs.cpp
reames requested changes to this revision.Oct 8 2015, 10:43 AM
reames edited edge metadata.

Meta comment: I really don't feel like FunctionAttrs is the right place to start. You need to work through a lot of the API questions and there are far simpler passes to work from. In particular, EarlyCSE would let you write test cases for most of the aliasing properties you're changing.

Second meta: The fact that you need to change *any* pass should seem really really suspicious. The most conservative implementation shouldn't need pass specific hooks.

include/llvm/IR/InstrTypes.h
1201

This appears to be a NFCI refactoring. Separate the submit without further review.

1234

I'd add a comment here about how individual bundles might refine this later.

Also, not sure this is the API we want. Would it be better to get the containing bundle, then ask *it* whether the use is capturing? Same for each of the predicated below.

include/llvm/IR/Instructions.h
1747

The second part of this check should be redundant.

1751

You appear to be going for the semantic that a bundle can override a function attribute, but not a call site attribute? If so, maybe a comment. This also needs clarified in the LangRef.

lib/Analysis/CaptureTracking.cpp
244

I don't think this line is needed currently since an unknown bundle will fail the following check. Please let whoever adds a non-capturing bundle worry about this.

lib/IR/Instructions.cpp
567

Duplicate code with above?

lib/Transforms/IPO/FunctionAttrs.cpp
131

This looks suspicious. We should be able to take the CS path. If we can't, I suspect you're missing semantically required hooks in AA, specifically, probably getModRefBehavior? Unless you're claiming that bundles prevent us from using the call within SCC reasoning?

437

See previous comment about future bundle author.

This revision now requires changes to proceed.Oct 8 2015, 10:43 AM
sanjoy added a comment.Oct 8 2015, 1:10 PM

(I'll reply to your inline review comments individually in some time, this is reply to the meta questions you raised)

Meta comment: I really don't feel like FunctionAttrs is the right place to start. You need to work through a lot of the API questions and there are far simpler passes to work from. In particular, EarlyCSE would let you write test cases for most of the aliasing properties you're changing.

FunctionAttrs seemed more "direct" as that lets me check the
inferred directly. I will soon start auditing other places in LLVM,
and I check those in first if you prefer.

Second meta: The fact that you need to change *any* pass should seem really really suspicious. The most conservative implementation shouldn't need pass specific hooks.

I'll reply to your inline comments individually, but generally the
problematic case (I'll make this clearer in the code) is when you have
things like

for (Value *A : CS.getArgs()) {
  if (!mayAlias(A, ThingIAmInterestedIn) && A->isReadOnlyArg())
    MayClobberThingIAmInterestedIn = true;
}

if (!MayClobberThingIAmInterestedIn)
  ...

An operand bundle use will simply not show up in CS.getArgs, and as
I write this I realize that I should really be fixing the loop to
iterate over all inputs to the call instruction including operand bundles

  • I'll do that restructuring.
include/llvm/IR/Instructions.h
1751

TBQH, that issue is not settled in my mind yet. I was surprised to
see that virtually nothing in LLVM "oversteps" the attributes in
CallSite to get the attributes directly from the Function * so
that may make implementing this scheme (calls can read/write even if
what they call is readnone) easier than it first looked.

My plan here is to keep this patch and patches based off of this out
of tree until I'm reasonably certain I'm not missing something
fundamental (in practice this means getting our internal workloads
successfully keeping deopt state in operand bundles through a significant
portion of pipeline).

reames added inline comments.Oct 8 2015, 2:18 PM
include/llvm/IR/Instructions.h
1751

I would *strongly* encourage you to improve existing interfaces where required to hide the details of operand bundles. Every pass change should have a very high bar because it greatly increases the odds of future errors.

I would also encourage you *not* to keep patches out of tree. You should write test cases for the most general possible bundle and check in each test. I agree that getting out of tree deopt bundle working is useful, but I feel the advantage of being able to work incrementally and collaborate with other ongoing work substantially outweighs the benefit of prototyping everything.

sanjoy abandoned this revision.Oct 21 2015, 4:16 PM

This has been superseded by D13961 and D13962.