This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Consolidate six getPointerOperand() utility functions into one place
ClosedPublic

Authored by hsaito on Feb 14 2018, 3:26 PM.

Details

Summary

See http://lists.llvm.org/pipermail/llvm-dev/2018-February/120999.html for llvm-dev discussions

There are six separate instances of getPointerOperand() utility. LoopVectorize.cpp has one of them,
and I don't want to create a 7th one while I'm trying to move LoopVectorizationLegality into a separate file
(eventual objective is to move it to Analysis tree).

I won't insist in having the utility in Instructions.h. Creating include/llvm/IR/InstructionUtils.h is a viable alternative to consider.

Diff Detail

Event Timeline

hsaito created this revision.Feb 14 2018, 3:26 PM

I like this change. Please also check whether Polly needs a similar change.
Thanks!

Mostly looks OK, just few minor comment.

lib/Analysis/DependenceAnalysis.cpp
3169–3172

Its good to assert with a message, i.e. "Instruction is not load or store Instruction"

3297–3300

Please add assert message !

lib/Transforms/Vectorize/LoopVectorize.cpp
5572–5573

need to correct indentation

hsaito added a comment.EditedFeb 16 2018, 11:04 AM

Mostly looks OK, just few minor comment.

Will do.

Line too long at 5572, right? Do you also like me to change the indentation at line 5570?
For the time being, I'll fix the long line.

Whose LGTM should I get to change Instructions.h or to create InstructionUtils.h? I'll invite Chris/Chandler for review, but I understand they are super busy with so many things.....

hsaito updated this revision to Diff 134701.Feb 16 2018, 12:54 PM

Also fixed another long line.

hsaito updated this revision to Diff 134710.Feb 16 2018, 1:32 PM

Assert fix missed in the last revision.

hsaito added inline comments.Feb 16 2018, 1:34 PM
lib/Analysis/DependenceAnalysis.cpp
3169–3172

Done

3297–3300

Done.

lib/Transforms/Vectorize/LoopVectorize.cpp
5572–5573

Fixed long line. Or did you mean the indentation at line 5570?

Looks like this is applicable to Polly also.

Polly ScopHelper.h
MemAccInst::getPointerOperand()

llvm::Value *getPointerOperand() const { 
  if (isLoad())
    return asLoad()->getPointerOperand();
  if (isStore())
    return asStore()->getPointerOperand();
  if (isMemIntrinsic()) 
    return asMemIntrinsic()->getRawDest();
  if (isCallInst())
    return nullptr;
  llvm_unreachable("Operation not supported on nullptr"); 
}

Very nice cleanup. I would be glad to use in Polly as well.

rengolin accepted this revision.Mar 9 2018, 1:11 AM
rengolin added a subscriber: rengolin.

I agree this is a nice cleanup, too.

Being a move to Instructions.h is a big deal.

But given that this commons up many independent components, and that this code truly belongs to that header file (and it's not such a big change), and that other maintainers and developers have acknowledge it, I'm going out on a limb and approving this patch.

We can always revert if it causes trouble, but I'm not expecting any.

cheers,
--renato

This revision is now accepted and ready to land.Mar 9 2018, 1:11 AM

Hi Hideki,

I tried to apply this patch but it failed in a number of places. Can you update your tree and submit again, please?

--renato

sebpop accepted this revision.Mar 9 2018, 12:27 PM

LGTM.
Thanks!

rengolin closed this revision.Mar 9 2018, 1:09 PM