This is an archive of the discontinued LLVM Phabricator instance.

[MemDep] Handle gep with zeros for invariant.group
ClosedPublic

Authored by Prazek on Dec 27 2016, 11:19 AM.

Details

Summary

gep 0, 0 is equivalent to bitcast. LLVM canonicalizes it
to getelementptr because it make SROA can then handle it.

Simple case like

void g(A &a) {
  z(a);
  if (glob)
      a.foo();
}
void testG() {
  A a;
  g(a);
}

was not devirtualized with -fstrict-vtable-pointers because luck of
handling for gep 0 in Memory Dependence Analysis

Event Timeline

Prazek updated this revision to Diff 82552.Dec 27 2016, 11:19 AM
Prazek retitled this revision from to [MemDep] Handle gep with zeros for invariant.group.
Prazek updated this object.
Prazek added reviewers: dberlin, nlewycky, chandlerc.
Prazek added a subscriber: llvm-commits.
Prazek updated this revision to Diff 82553.Dec 27 2016, 11:25 AM

Updated documentation

Prazek updated this revision to Diff 82555.Dec 27 2016, 11:28 AM

Fixed comment

chandlerc edited edge metadata.Dec 27 2016, 11:30 AM

While I think this patch is simple and a nice pragmatic improvement, I don't want to prematurely cut off discussion about whether we have the right canonicalization here. Maybe leave a FIXME about the fact that we have two representations that are used? Unsure

I suspect something like this should go in at least while instcombine is actively canonicalizing in this direction just for practical reasons.

lib/Analysis/MemoryDependenceAnalysis.cpp
354–362

Is the adding of const everywhere helping much? We've historically avoided it, but I have no really strong opinions...

356–359

I think its fine to just use [&] capture on simple factorings like this.

383–384

It would be nice to avoid going through the type lookup machinery twice here.

Prazek added inline comments.Dec 27 2016, 12:21 PM
lib/Analysis/MemoryDependenceAnalysis.cpp
354–362

I only added 4 consts, where 2 are inside container and 2 in the loop (which could be avoided by using auto, but in that cases it seems cleaner without auto).

I prefer const in the container types because I can show this way that I am not going to modify any of these values

383–384

Code elegance your concern here right?
I can put it in ifs like

if (isa<BitCastInst>(U)) {
  TryInsertToQueue(U);
  continue;
}
if (auto *GEP = dyn_cast<GetElementPtr>(U)) {
  if(GEP->hasAllZeroIndices())
    TryInsertToQueue(U);
  continue;
}

I am not sure if it will be cleaner (specially because of nested if)

Prazek added inline comments.Dec 27 2016, 12:35 PM
lib/Analysis/MemoryDependenceAnalysis.cpp
383–384

if (auto *GEP = dyn_cast<GetElementPtr>(U); GEP && GEP->hasAllZeroIndices()) {

TryInsertToQueue(U);
continue;

}

This is exactly why I really would love to use C++17 in LLVM :)

JDevlieghere added inline comments.
lib/Analysis/MemoryDependenceAnalysis.cpp
383–384

Alternatively you could switch on U->getOpcode() and use a regular cast. This way you have to check the type only once, at the expense of adding more clutter. Wouldn't do it here though.

Prazek marked an inline comment as done.Dec 27 2016, 2:14 PM
Prazek added inline comments.
lib/Analysis/MemoryDependenceAnalysis.cpp
383–384

I am pretty sure that clang is smart enough to do the exact same thing (all the functions are inline so it is not that hard)

Prazek updated this revision to Diff 82561.Dec 27 2016, 2:20 PM
Prazek edited edge metadata.

Added comments and & in lambda

Prazek marked 7 inline comments as done.Dec 28 2016, 8:30 AM
Prazek updated this revision to Diff 82597.Dec 28 2016, 8:31 AM

Addressing Chandler's comments

Prazek updated this object.Dec 28 2016, 8:35 AM
Prazek set the repository for this revision to rL LLVM.
Prazek added a subscriber: davide.
Prazek updated this revision to Diff 82609.Dec 28 2016, 10:19 AM
  • small fix

I am not really sure who is the code owner of MemDep

reames added inline comments.Dec 29 2016, 4:06 PM
lib/Analysis/MemoryDependenceAnalysis.cpp
390

minor: remove brackets for single statement ifs.

test/Transforms/GVN/invariant.group.ll
323

you need a few more check statements here. As written, I'm not sure what this test is testing by just reading it.

Also, can you strip away any of this?

Prazek added inline comments.Dec 30 2016, 1:05 AM
lib/Analysis/MemoryDependenceAnalysis.cpp
390

sure

test/Transforms/GVN/invariant.group.ll
323

Strip away what?

Test checks if invariant.group works also on zero geps (%2).
You are right, I need at least check of
call void @_ZN1A3fooEv(%struct.A* nonnull dereferenceable(8) %a)

Prazek updated this revision to Diff 82723.Dec 30 2016, 4:13 AM
Prazek marked 4 inline comments as done.
  • small fix
  • address Phillip's comments
Prazek added a subscriber: vsk.Dec 30 2016, 7:23 AM
reames accepted this revision.Dec 30 2016, 9:52 AM
reames added a reviewer: reames.

LGTM

lib/Analysis/MemoryDependenceAnalysis.cpp
376

minor: walking back the def chain could be handled by a call to stripPointerCasts. This can be a following cleanup change,

This revision is now accepted and ready to land.Dec 30 2016, 9:52 AM

Thanks :)

lib/Analysis/MemoryDependenceAnalysis.cpp
376

do you mean firstly call stripPointerCasts on the LoadOperand so that we will only have to walk down the tree? I didn't know this function, but I was also thinking about splitting it into 2 loops to firstly get to the top.

This revision was automatically updated to reflect the committed changes.
Prazek marked an inline comment as done.Dec 30 2016, 12:26 PM