This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add getAccessType to Instruction
ClosedPublic

Authored by luke on May 15 2023, 9:01 AM.

Details

Summary

There are multiple places in the code where the type of memory being accessed from an instruction needs to be obtained, including an upcoming patch to improve GEP cost modeling. This deduplicates the logic between them. It's not strictly NFC as EarlyCSE/LoopStrengthReduce may catch more intrinsics now.

Diff Detail

Event Timeline

luke created this revision.May 15 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 9:01 AM
luke requested review of this revision.May 15 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 9:01 AM

Please can you add examples of its use (either in this patch or a follow child patch)?

luke added a comment.May 15 2023, 9:17 AM

Please can you add examples of its use (either in this patch or a follow child patch)?

Hi there, didn't expect a review so quickly :) the child patch should be attached now.

The other places that I've seen use this are loopstrengthreduce and earlyCSE.

There's also the question as to whether or not this should subsume Type *getLoadStoreType(Value *I), also defined in Instructions.h.

nikic added a comment.May 16 2023, 1:02 AM

Could you please also replace some reimplementations of this functionality with the helper, to show that it is generically useful?

llvm/lib/IR/Instruction.cpp
749

getPointerOperand()

753

This is incorrect for cmpxchg, which returns a struct.

luke updated this revision to Diff 522565.May 16 2023, 5:58 AM

Fix AtomicCmpXChg type, use in EarlyCSE + LoopStrengthReduce

luke marked 2 inline comments as done.May 16 2023, 6:01 AM
luke added inline comments.
llvm/lib/IR/Instruction.cpp
753

Thanks, good catch. I've plugged this into LoopStrengthReduce, which seems to have made the same mistake, so this patch is no longer NFC.

luke retitled this revision from [IR] Add getAccessType to Instruction. NFC to [IR] Add getAccessType to Instruction.May 16 2023, 6:02 AM
luke edited the summary of this revision. (Show Details)
luke marked an inline comment as done.
nikic added inline comments.May 16 2023, 6:07 AM
llvm/lib/IR/Instruction.cpp
749

Sorry, I meant to say getValueOperand() here...

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
900 ↗(On Diff #522565)

Is this assertion right? After all we're also handling memcpy etc below.

luke updated this revision to Diff 522579.May 16 2023, 6:22 AM
luke edited the summary of this revision. (Show Details)

Address comments

nikic added inline comments.May 28 2023, 12:25 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
920 ↗(On Diff #522579)

Is the drop of the MemTy assignment here intentional?

luke updated this revision to Diff 533242.Jun 21 2023, 6:41 AM
luke marked an inline comment as done.

Address review comments, remove unnecessary diff

luke added inline comments.Jun 21 2023, 6:43 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
920 ↗(On Diff #522579)

Doesn't look like it. I've removed the unnecessary diffs, sorry about that.

luke marked 2 inline comments as done.Jun 21 2023, 6:43 AM
nikic accepted this revision.Jun 21 2023, 6:45 AM

LGTM

This revision is now accepted and ready to land.Jun 21 2023, 6:45 AM
This revision was landed with ongoing or failed builds.Jun 21 2023, 8:17 AM
This revision was automatically updated to reflect the committed changes.