This is an archive of the discontinued LLVM Phabricator instance.

[PATCH 06/27] [noalias] [IR] IRBuilder support for noalias intrinsics.
Needs ReviewPublic

Authored by jeroen.dobbelaere on Oct 4 2019, 2:28 PM.

Details

Reviewers
hfinkel
jdoerfert
Summary

This is part of the series started by D68484.

Note: this is a stable point and tests should run fine with the patches applied up to this point.
Note: D68490 has been incorporated

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 2:28 PM
lebedev.ri added inline comments.
llvm/include/llvm/IR/IRBuilder.h
651

But ObjId is uint64_t?

654

Here and elsewhere - currently llvm variable naming scheme did not change, all variables start with Uppercase Letter.

663

Should this be uint32_t ?

671

Should this be uint32_t ?

llvm/include/llvm/IR/IntrinsicInst.h
889–890

assert(lhs->getIntrinsicID() == rhs->getIntrinsicID() && ;hs->getIntrinsicID() == Intrinsic::side_noalias && "Can only check noalias compatibility of side_noalias intrinsics");

891–896

std::tie(<...>) == std::tie(<...>) ?

llvm/lib/IR/IRBuilder.cpp
514

Should this be uint64_t?

565–566

Ops.insert(Ops.end(), MDNodes.begin(), MDNodes.end()) ?

569

same

573–577

This should be in the parent patch that added that

llvm/lib/IR/Verifier.cpp
4760–4761
} else if (auto *PHI = dyn_cast<PHINode>(V)) {
4762–4763

Worklist.insert() ?

jeroen.dobbelaere marked 5 inline comments as done.Oct 7 2019, 3:41 AM
jeroen.dobbelaere added inline comments.
llvm/include/llvm/IR/IRBuilder.h
651

I am preparing an update where I use uint64_t more consistenly.

llvm/include/llvm/IR/IntrinsicInst.h
891–896

You mean: std::forward_as_tuple ?

llvm/lib/IR/IRBuilder.cpp
514

No. It should be int64_t.

565–566

That won't work.

569

But here it will.

573–577

Maybe. I tried to not do that kind of changes in the rebased versions.

Treat objId as uint64_t everywhere; treat indices as int64_t; use std::forward_as_tuple; .use insert instead of for-loop+push_back.

jeroen.dobbelaere marked 12 inline comments as done.Oct 7 2019, 7:49 AM
vermaelen.wouter added inline comments.
llvm/include/llvm/IR/IntrinsicInst.h
891–896

Doesn't that loose the short-circuit behavior of operator&&?

Detail: IMHO it doesn't really improve readability:

(a == b) && (c == d)

versus

std::tie(a, c) == std::tie(b, d);
jeroen.dobbelaere marked an inline comment as done.Oct 28 2019, 9:18 AM

Yes, using forward_as_tuple might result in slower code... I am reverting that change.

Do not use std::forward_as_tuple.

jeroen.dobbelaere retitled this revision from [PATCH 08/38] [noalias] [IR] IRBuilder support for noalias intrinsics. to [PATCH 05/26] [noalias] [IR] IRBuilder support for noalias intrinsics..
jeroen.dobbelaere edited the summary of this revision. (Show Details)
jeroen.dobbelaere retitled this revision from [PATCH 05/26] [noalias] [IR] IRBuilder support for noalias intrinsics. to [PATCH 06/27] [noalias] [IR] IRBuilder support for noalias intrinsics..

Rebased to 9fb46a452d4e5666828c95610ceac8dcd9e4ce16 (September 7, 2020)