This is an archive of the discontinued LLVM Phabricator instance.

[ImplicitNullCheck] Add alias analysis usage
ClosedPublic

Authored by skatkov on Feb 24 2017, 3:57 AM.

Details

Summary

With this change ImplicitNullCheck optimization uses alias analysis and
can use load/store memory access for implicit null check if there are other
load/store before but memory accesses do not alias.

Diff Detail

Event Timeline

skatkov created this revision.Feb 24 2017, 3:57 AM
sanjoy requested changes to this revision.Feb 26 2017, 3:21 PM

Some comments inline.

lib/CodeGen/ImplicitNullChecks.cpp
367–378

s/alias/aliases/

373

Can we pull the body of this loop out into a static helper?

379

Can we return SR_Impossible here if the MachineInstr that was the store also has no mem operands?

386

The indents are off -- please use clang-format.

395

Maybe move this check up to avoid wasting work if AA isn't available?

Btw, given the way you're getting AA, I'd expect it always be present since otherwise getAnalysis<AAResultsWrapperPass>().getAAResults() will have to be a null reference which is undefined behavior.

402

What about multiple memoperands on the same machine instruction?

This revision now requires changes to proceed.Feb 26 2017, 3:21 PM

Will prepapre an update.

lib/CodeGen/ImplicitNullChecks.cpp
367–378

ok

373

will do

379

makes sense.

386

sure

395

we can be in a good shape even if AA is not available. Say if other instruction is a spill. See the previous check: PSV != null and PSV->mayAlias is false. So it is not good to move AA check earlier. WRT AA is not null I will double check your idea.

402

Not sure it is possible but we can iterate over mem operands.

sanjoy added inline comments.Feb 26 2017, 8:21 PM
lib/CodeGen/ImplicitNullChecks.cpp
402

I'd also be happy with either of:

  • An assert that says that there is only one mem operand
  • A bailout if there are more than one mem operand (if they are very rare)
skatkov updated this revision to Diff 89835.Feb 26 2017, 9:02 PM
skatkov edited edge metadata.

Address comments. Please review again.

skatkov marked 13 inline comments as done.Feb 26 2017, 9:03 PM
sanjoy requested changes to this revision.Feb 27 2017, 5:01 PM

Comments inline.

lib/CodeGen/ImplicitNullChecks.cpp
340

Can we name this as a verb, like areMemoryOpsAliased?

357

This clause is a bit odd -- without this clause, the nested loop returns SR_Suitable only if all pairs of mem operands are NoAlias. But this clause returns SR_Suitable even if one of PrevMI mem operands is a PseudoSourceValue with !PSV->mayAlias(MFI). Why do we have this difference? If we have to, I'd prefer checking for PseudoSourceValue in a loop only over PrevMI->memoperands() outside this nested loop.

Also: can one of MI.memoperands() be a PseudoSourceValue? If not, please add an assert.

366

Spacing is off here, but I'll run clang-format on the diff before pushing it so don't bother fixing it right now.

369

s/SR_impossible/SR_Impossible/

371

This layering seems odd to me -- AliasMemoryOp should not return a SuitabilityResult IMO, since it has no idea on why it has been asked to compute aliasing between the two instructions. It almost feels like it should return a tri-stated AliasResult with AR_NoAlias, AR_MayAlias and AR_WillAliasEverything. That would be a bit more code, but I think the intent will be cleaner.

This revision now requires changes to proceed.Feb 27 2017, 5:01 PM
skatkov updated this revision to Diff 89967.Feb 27 2017, 8:09 PM
skatkov edited edge metadata.

addressed comments, please review.

Also I've run clang-format against ImplicitNullChecks.cpp and it shows a bit other issues not directly related to my change.
Specifically it does not like one-line enum. If you'd like I can fix it as well.

skatkov marked 5 inline comments as done.Feb 27 2017, 8:10 PM
skatkov added inline comments.
lib/CodeGen/ImplicitNullChecks.cpp
357

Good catch, fixed.

371

Sounds reasonable to me.

addressed comments, please review.

Also I've run clang-format against ImplicitNullChecks.cpp and it shows a bit other issues not directly related to my change.

You can use git clang-format (after installing https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format) to format just code touched by a specific commit. For instance, to format the code touched in the most recent commit, you can do git clang-format HEAD^.

Specifically it does not like one-line enum. If you'd like I can fix it as well.

sanjoy accepted this revision.Feb 27 2017, 9:31 PM

lgtm with really minor tweaks

lib/CodeGen/ImplicitNullChecks.cpp
168

The ImplicitNullChecks:: is not needed here, is it?

Also, I'd prefer plural here, as areMemoryOpsAliased.

309

s/if/If/

324

No need for the else. That is, you can say:

if (...)
  return ...;
continue;
This revision is now accepted and ready to land.Feb 27 2017, 9:31 PM
skatkov updated this revision to Diff 89975.Feb 27 2017, 10:40 PM
skatkov marked 2 inline comments as done.

Sanjoy, please land if you are ok with this.

skatkov marked 5 inline comments as done.Feb 27 2017, 10:42 PM
This revision was automatically updated to reflect the committed changes.