This is an archive of the discontinued LLVM Phabricator instance.

invariant.group handling in GVN
ClosedPublic

Authored by Prazek on Sep 18 2015, 7:12 PM.

Details

Summary

The most important part required to make clang

devirtualization works ( ͡°͜ʖ ͡°).
The code is able to find non local dependencies, but unfortunatelly
because the caller can only handle local dependencies, I had to add
some restrictions to look for dependencies only in the same

Diff Detail

Event Timeline

Prazek updated this revision to Diff 35154.Sep 18 2015, 7:12 PM
Prazek retitled this revision from to invariant.group handling in GVN.
Prazek updated this object.
Prazek added reviewers: rsmith, nlewycky, majnemer.
Prazek added a subscriber: llvm-commits.
rsmith added inline comments.Sep 23 2015, 10:41 AM
include/llvm/Analysis/MemoryDependenceAnalysis.h
420

Typo "Incariant"; use \brief insteaed of repeating the function name here.

422

will -> does

423

loads or stores -> load or store

lib/Analysis/MemoryDependenceAnalysis.cpp
379

Revert this blank line insertion.

lib/Transforms/Utils/Local.cpp
1433

instruction has -> instructions have

1434

Maybe add a FIXME stating that we should try to preserve both groups in this case.

dberlin added inline comments.
lib/Analysis/MemoryDependenceAnalysis.cpp
402

Well, it's safe, if you verify the use is in LI->getParent()->getParent(), no?

Prazek marked 6 inline comments as done.Sep 27 2015, 9:47 PM
Prazek added inline comments.
lib/Analysis/MemoryDependenceAnalysis.cpp
402

Yes, but nobody guarantee that uses list won't change when I will iterate over it.

Prazek updated this revision to Diff 35828.Sep 27 2015, 9:52 PM
Prazek updated this revision to Diff 35903.Sep 28 2015, 1:35 PM

Forgot to include test!

Prazek updated this revision to Diff 36013.Sep 29 2015, 11:29 AM

Fixed combine metadata

nlewycky added inline comments.Sep 29 2015, 2:21 PM
include/llvm/Analysis/MemoryDependenceAnalysis.h
399–400

I don't think that's "brief". "[A] brief description is a short one-liner" -- https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html

420

Brief again.

lib/Analysis/MemoryDependenceAnalysis.cpp
399

"use list" not "uses list" (two occurrences in this line).

402

It's the 'function passes aren't allowed to look outside their functions' rule, as-if we had function passes running in parallel.

419

Hold up. If there's three loads, A B and C, all of them in the same invariant group, and C is the QueryInst for getPointerDependencyFrom, and A and B both dominate C, doesn't the value returned right here depend on the order of the use list? That seems undesirable, but I'm not sure whether that's considered a serious problem or not.

lib/Transforms/Scalar/GVN.cpp
2480

Spurious change

lib/Transforms/Utils/Local.cpp
1440–1441

Just move this part into the "case LLVMContext::MD_invariant_group:" ?

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

Space after colon

Prazek updated this revision to Diff 36063.Sep 29 2015, 8:32 PM

You call it devirtualization? This is devirtualization

Prazek added inline comments.Sep 29 2015, 8:35 PM
lib/Analysis/MemoryDependenceAnalysis.cpp
419

it doesn't matter if we will return Def for A or B, because they will load the same value. The second thing is that we will process them in order which they are in the block, which means that for A we will do nothing, for B we will return Def(A) and remove B, and for C the same thing (because B will not longer exist)

lib/Transforms/Utils/Local.cpp
1440–1441

It won't work because we iterate over K metadata

Prazek updated this revision to Diff 36064.Sep 29 2015, 8:48 PM
Prazek marked an inline comment as done.

reformated

Prazek updated this revision to Diff 36065.Sep 29 2015, 9:11 PM
Prazek added a reviewer: dberlin.
Prazek marked 6 inline comments as done.

small fixes

Prazek updated this revision to Diff 36131.Sep 30 2015, 11:13 AM
Prazek updated this revision to Diff 36184.Sep 30 2015, 9:46 PM
Prazek updated this object.

Works only on single block, I don't think I will make it to work non locally

nlewycky accepted this revision.Oct 1 2015, 12:59 PM
nlewycky edited edge metadata.

LGTM, thanks for the extensive gvn test!

include/llvm/Analysis/MemoryDependenceAnalysis.h
422

... if *it* does not find ...

426

Either "on the caller" or "at the call site".

lib/Analysis/MemoryDependenceAnalysis.cpp
398

Extra newline.

411

"equivalent"

This revision is now accepted and ready to land.Oct 1 2015, 12:59 PM
Prazek updated this revision to Diff 36377.Oct 2 2015, 10:42 AM
Prazek updated this object.
Prazek edited edge metadata.
Prazek marked 4 inline comments as done.
Prazek closed this revision.Oct 2 2015, 3:26 PM