This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Introduce a type to model memory operation
ClosedPublic

Authored by gchatelet on Jan 31 2020, 6:41 AM.

Details

Summary

This is a first step before changing the types to llvm::Align and introduce functions to ease client code.

Event Timeline

gchatelet created this revision.Jan 31 2020, 6:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2020, 6:41 AM
arsenm added inline comments.Jan 31 2020, 6:45 AM
llvm/include/llvm/CodeGen/TargetLowering.h
110

Needs src and dst address spaces

gchatelet marked 2 inline comments as done.Jan 31 2020, 7:02 AM
gchatelet added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
110

So address spaces are useful at the findOptimalMemOpLowering level but not at lower levels (getOptimalMemOpType, getOptimalMemOpLLT) so I was not sure whether it would make sense to include them. I don't have a strong opinion at that point. courbet@ WDYT?

courbet added inline comments.Jan 31 2020, 7:02 AM
llvm/include/llvm/CodeGen/TargetLowering.h
110

Some operations (e.g. getOptimalMemOpType) do not take an address space.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
2683

spurious reformatting, can you please revert this to facilitate the review ?

gchatelet updated this revision to Diff 241729.Jan 31 2020, 7:24 AM
gchatelet marked 2 inline comments as done.
  • Revert spurious formatting
gchatelet marked 2 inline comments as done.Jan 31 2020, 7:26 AM
gchatelet added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
110

@arsenm do you think it would make sense to propagate the address spaces down to the TargetLowering implementations? Is it a bug so to speak that we always consider address spaces to be compatible?

courbet added inline comments.Jan 31 2020, 7:41 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5910–5911

Could you submit the fix to the broken isZeroConstant code as a separate patch so that this remains an NFC ?

gchatelet updated this revision to Diff 241738.Jan 31 2020, 8:10 AM
gchatelet marked an inline comment as done.
  • Remove fix so it's indeed NFC
courbet accepted this revision.Jan 31 2020, 8:27 AM
This revision is now accepted and ready to land.Jan 31 2020, 8:27 AM
gchatelet marked an inline comment as done.Jan 31 2020, 8:28 AM
gchatelet added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
110

Upon careful analysis, only DstAS is used as a fallback in case we don't have an optimal type to use so I don't think it matters to pass them down.
Also SrcAs is unused in both function so I'll create a subsequent patch to remove this argument. Thx for the heads up though @arsenm .

This revision was automatically updated to reflect the committed changes.