Page MenuHomePhabricator

[Polly] [Refactor] Enable llvm's isa/cast/dyn_cast on MemAccInst
ClosedPublic

Authored by etherzhhb on Feb 14 2016, 4:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

etherzhhb updated this revision to Diff 47939.Feb 14 2016, 4:28 PM
etherzhhb retitled this revision from to [Polly] [Refactor] Make MemAccInst more concise..
etherzhhb updated this object.
etherzhhb added reviewers: Meinersbur, grosser.
etherzhhb set the repository for this revision to rL LLVM.
etherzhhb added a subscriber: Restricted Project.
Meinersbur added inline comments.Feb 15 2016, 2:28 AM
include/polly/Support/ScopHelper.h
81 ↗(On Diff #47939)

The distinction between implicit an explicit constructors is lost with this. In particular, I'd avoid an arbitrary Instruction to be converted to MemAccInst without a kind of explicit 'cast'. This makes a execution-time error out of it, including if passing someting that is not even an Instruction.

86 ↗(On Diff #47939)

classof is intended for LLVM's object hierarchy and MemAccInst is no part of it. It is not derived from llvm::Value or similar. That means eg.

LoadInst *LI;
cast<MemAccInst>(LI)

will go horribly wrong.

97 ↗(On Diff #47939)

This scheme doesn't work in general when we want something implicitly converted to the argument. Though, in this case, because nothing derives from LoadInst or StoreInst, there might be no such case. I'd still prefer the previous for readability.

102 ↗(On Diff #47939)

No StaticAssertLegalT here?

106 ↗(On Diff #47939)

That's want I suggested an planned as well.

115 ↗(On Diff #47939)

Is there any advantage of this scheme that two ifs? Both have 5 lines.

126 ↗(On Diff #47939)

I'd rather avoid using macros, but still OK.

147 ↗(On Diff #47939)

Wow, I didn't know this is possible.

Meinersbur edited edge metadata.Feb 15 2016, 2:33 AM

I like the operator->() and simplify_type, but am sceptic about the rest.

Will refine the patch

include/polly/Support/ScopHelper.h
81 ↗(On Diff #47939)

ok

86 ↗(On Diff #47939)

It works if we introduce simplify_type, and I actually experimented this and confirm it work.

I think it should be

return I && (llvm::isa<llvm::LoadInst>(I) || llvm::isa<llvm::StoreInst>(I));
97 ↗(On Diff #47939)

ok

102 ↗(On Diff #47939)

We call "operator=(T *)" which has the StaticAssertLegalT

115 ↗(On Diff #47939)

I dont think it has advantage, this together with the following macro are copied from CallSite... I first copy here then copy to the following marco

126 ↗(On Diff #47939)

We may think about using template to do the same thing when we have time.

grosser resigned from this revision.Feb 25 2016, 5:20 AM
grosser removed a reviewer: grosser.

I leave these patch to you guys.

Still working in it? Due to MemIntrinsics, the macroization/templatization doesn't work anymore?.?

Still working in it? Due to MemIntrinsics, the macroization/templatization doesn't work anymore?.?

Originally, I actually waiting @jdoerfert to checkin his patches and work on it. Now I can continue

Sorry for the dealy. I didn't want to block you guys.

not blocking, I just think this one is not as high priority as yours, so I
just wait to avoid unnecessary rebase on your side

etherzhhb updated this revision to Diff 49190.Feb 26 2016, 8:00 AM
etherzhhb retitled this revision from [Polly] [Refactor] Make MemAccInst more concise. to [Polly] [Refactor] Enable llvm's isa/cast/dyn_cast on MemAccInst.
etherzhhb edited edge metadata.

Updated

Meinersbur accepted this revision.Feb 26 2016, 8:49 AM
Meinersbur edited edge metadata.

LGTM

lib/Analysis/ScopInfo.cpp
619 ↗(On Diff #49190)

Does this work was well?

assert(isa<MemIntrinsic>(getAccessInstruction()));
This revision is now accepted and ready to land.Feb 26 2016, 8:49 AM
etherzhhb added inline comments.Feb 26 2016, 9:01 AM
lib/Analysis/ScopInfo.cpp
619 ↗(On Diff #49190)

if getAccessInstruction() return nullptr.

assert(isa<MemIntrinsic>(MemAccInst(getAccessInstruction())));

Trigger an assertion failure.

assert(isa<MemIntrinsic>(getAccessInstruction()));

Trigger a SIGSEGV (suppose to ... no sure)

Maybe we should write:

assert(getAccessInstruction() && isa<MemIntrinsic>(getAccessInstruction()));
Meinersbur added inline comments.Feb 26 2016, 9:08 AM
lib/Analysis/ScopInfo.cpp
619 ↗(On Diff #49190)

isa<XYZ*>(nullptr) triggers the following assertion:

assert(Val && "isa<> used on a null pointer");

in llvm/Support/Casting.h:81/88/95/102

etherzhhb added inline comments.Feb 26 2016, 9:13 AM
lib/Analysis/ScopInfo.cpp
619 ↗(On Diff #49190)

ok

This revision was automatically updated to reflect the committed changes.