Page MenuHomePhabricator

Fix aliasing of launder.invariant.group
ClosedPublic

Authored by Prazek on May 18 2018, 3:05 PM.

Details

Summary

Patch for capture tracking broke
bootstrap of clang with -fstict-vtable-pointers
which resulted in debbugging nightmare. It was fixed
https://reviews.llvm.org/D46900 but as it turned
out, there were other parts like inliner (computing of
noalias metadata) that I found after bootstraping with enabled
assertions.

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek created this revision.May 18 2018, 3:05 PM
kuhar added inline comments.May 18 2018, 3:14 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
135 ↗(On Diff #147597)

Why was the previous change reverted here?

llvm/test/Transforms/Inline/launder.invariant.group.ll
5 ↗(On Diff #147597)

nit: its

Prazek marked an inline comment as done.May 18 2018, 3:27 PM
Prazek added inline comments.
llvm/lib/Analysis/BasicAliasAnalysis.cpp
135 ↗(On Diff #147597)

Because it is not needed now, check comment I left on line 1625

1625 ↗(On Diff #147597)

We never go to this branch, because O1 and O2 are the same by changing GetUnderlyingObject

kuhar added inline comments.May 18 2018, 3:29 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
135 ↗(On Diff #147597)

Makes sense, thanks

Is there some general principle we can outline to describe where these updates are needed?

Prazek marked 4 inline comments as done.EditedMay 18 2018, 3:51 PM

Is there some general principle we can outline to describe where these updates are needed?

I am not sure how to do it right since I am not that familiar with this codebase. Maybe we can make function like

// This function can be used only for aliasing properties. You CAN'T use it to replace
// one value with another!
bool getArgumentAliasingToReturnedPointer(Function *F) {
  if (Value *RV = CS.getReturnedArgOperand())
      return RV;
  if (CS.getIntrinsicID() == Intrinsic::launder_invariant_group)
      return CS.getArgOperand(0);

  return nullptr;    
}

And use it in the places that I modified. Unfortunately it will not change CaptureTracking, because it goes top-down instead of bottom-up.

How does it sound to you?

Is there some general principle we can outline to describe where these updates are needed?

I am not sure how to do it right since I am not that familiar with this codebase. Maybe we can make function like

// This function can be used only for aliasing properties. You CAN'T use it to replace
// one value with another!
bool getArgumentAliasingToReturnedPointer(Function *F) {
  if (Value *RV = CS.getReturnedArgOperand())
      return RV;
  if (CS.getIntrinsicID() == Intrinsic::launder_invariant_group)
      return CS.getArgOperand(0);
 
  return nullptr;    
}

I think this refactoring makes sense. However...

And use it in the places that I modified.

What I want to know is: how did you decide what had to be modified? Did you just look for every place that handles returned arguments? If you had to describe to someone how to find the places that need modification, how would you do it? (because that's what we need in the comment).

Unfortunately it will not change CaptureTracking, because it goes top-down instead of bottom-up.

How does it sound to you?

Prazek marked an inline comment as done.May 18 2018, 4:39 PM

What I want to know is: how did you decide what had to be modified? Did you just look for every place that handles returned arguments? If you had to describe to someone how to find the places that need modification, how would you do it? (because that's what we need in the comment).

Yes, I pretty much checked every place that uses CaptureTracking and looked for special handling of calls or uses of getUnerlyingObject.

What I want to know is: how did you decide what had to be modified? Did you just look for every place that handles returned arguments? If you had to describe to someone how to find the places that need modification, how would you do it? (because that's what we need in the comment).

Yes, I pretty much checked every place that uses CaptureTracking and looked for special handling of calls or uses of getUnerlyingObject.

Okay.

Prazek updated this revision to Diff 147626.May 18 2018, 5:37 PM

Refactored code to getArgumentAliasingToReturnedPointer and added few comments.
Please check out isKnownNonZero, as I think it could also use getArgumentAliasingToReturnedPointer.

Some of these changes are required for correctness, right? For those, please specifically add a comment near those explaining why.

llvm/include/llvm/Analysis/ValueTracking.h
580 ↗(On Diff #147626)

Extra whitespace change?

Prazek updated this revision to Diff 147656.May 19 2018, 4:47 AM
Prazek marked an inline comment as done.

Added comments

amharc accepted this revision.May 19 2018, 5:00 AM

Of course, you should wait for an LGTM from someone more competent than me ;)

llvm/lib/Analysis/CaptureTracking.cpp
255 ↗(On Diff #147656)

nit: do not capture

This revision is now accepted and ready to land.May 19 2018, 5:00 AM
Prazek updated this revision to Diff 147657.May 19 2018, 5:12 AM
Prazek marked an inline comment as done.
  • Fixed nit
xbolva00 added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
3389 ↗(On Diff #147657)

Why assert here?

Prazek added inline comments.May 19 2018, 5:37 AM
llvm/lib/Analysis/ValueTracking.cpp
3389 ↗(On Diff #147657)

It is possible that this funciton would return nullptr if passed null CS,
but such function would be harder to reason about. In this case I assume that caller checked the the instruction is a CallSite, the same way as is done with different functions.

xbolva00 added inline comments.May 19 2018, 5:40 AM
llvm/lib/Analysis/ValueTracking.cpp
3389 ↗(On Diff #147657)

Can you add a info message into assert then?
assert (CS && "...");

Prazek updated this revision to Diff 147658.May 19 2018, 5:49 AM
Prazek marked 3 inline comments as done.
  • use getArgumentAliasingToReturnedPointer as it does not change behavior
llvm/lib/Analysis/ValueTracking.cpp
3389 ↗(On Diff #147657)

sure, make sense

xbolva00 accepted this revision.May 19 2018, 5:54 AM
hfinkel added inline comments.May 19 2018, 8:29 AM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
435 ↗(On Diff #147658)

Please explain here *how* this keeps this in sync. There's no obvious use of CaptureTracking here.

llvm/lib/Analysis/ValueTracking.cpp
3445 ↗(On Diff #147658)

Same here. Please explain how this helps to keep tings in sync with CaptureTracking. There's some implicit contract here between these functions that we need to document explicitly.

Prazek marked an inline comment as done.May 19 2018, 3:25 PM
Prazek marked 2 inline comments as done.May 19 2018, 4:03 PM
Prazek added inline comments.
llvm/lib/Analysis/BasicAliasAnalysis.cpp
435 ↗(On Diff #147658)

I updated the comment, but I have a new idea how to make it better. I will refactor the list of intrinsic to some other function, to make it always in sync.

Prazek updated this revision to Diff 147685.May 19 2018, 4:33 PM
Prazek marked an inline comment as done.
  • small refactor
llvm/lib/Analysis/ValueTracking.cpp
3399 ↗(On Diff #147685)

Note that llvm.strip.invariant.group will be added in this patch
https://reviews.llvm.org/D47103

Prazek updated this revision to Diff 147690.May 19 2018, 5:01 PM

fix name

hfinkel added inline comments.May 19 2018, 10:14 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
435 ↗(On Diff #147658)

That's good, but doesn't explain why keeping these in sync is necessary for correctness. To put it another way, please explain what does wrong if these are not in sync.

Prazek updated this revision to Diff 148139.May 22 2018, 5:57 PM
  • Add more comments
Prazek marked 2 inline comments as done.May 22 2018, 5:57 PM
hfinkel accepted this revision.May 22 2018, 6:08 PM

LGTM

This is still a bit convoluted, but I think that someone can now figure out what's going on from the comments. They also raise an interesting point of whether we should somehow separate out the capture-only-by-return case from the more-general case (although that's nothing to be addressed here).

This revision was automatically updated to reflect the committed changes.