Page MenuHomePhabricator

[Alignment][NFC] Migrate TargetTransformInfo::allowsMisalignedMemoryAccesses to Align
Needs ReviewPublic

Authored by gchatelet on Jun 30 2020, 7:00 AM.

Details

Reviewers
courbet
Summary

This patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Event Timeline

gchatelet created this revision.Jun 30 2020, 7:00 AM
gchatelet marked 2 inline comments as done.Jun 30 2020, 7:21 AM
gchatelet added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
898

[allowsMisalignedMemoryAccesses](https://github.com/llvm/llvm-project/blob/70f6389257a85a8fa7f128a05a1ccbd0dbba191c/llvm/include/llvm/CodeGen/TargetLowering.h#L1576) can only consider valid alignment. 0 here is not a valid value, that's why it is turned into a 1.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
252

ditto

courbet added inline comments.Jun 30 2020, 7:58 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
898

This is not necessarily and NFC (see my other comment below).

llvm/lib/Target/ARM/ARMISelLowering.cpp
16150

When alignment was 0, and v8i8, this is not an NFC.

gchatelet marked an inline comment as done.Jun 30 2020, 8:30 AM
gchatelet added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
16150

Ty == MVT::v4i8 || Ty == MVT::v8i8 || Ty == MVT::v4i16 so Ty is either 32 or 64 bits (v4i8 is 32, v8i8 and v4i16 are 64)
Since VT is a SimpleVT => VT.getScalarSizeInBits() can only be 32 or 64 as well.
Then VT.getScalarSizeInBits() / 8 can only be 4 or 8 so it doesn't matter whether Alignment is 0 or 1.

Or am I missing something?

courbet added inline comments.Jun 30 2020, 11:39 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
16150

isSimple means native to some processor (as opposed to extended). But e.g. MVT::v8i8 is both a simple and vector EVT. the scalar type for MVT::v8i8 is MVT::i8, so VT.getScalarSizeInBits()==8, i.e. VT.getScalarSizeInBits() / 8 == 1

gchatelet marked an inline comment as done.Jul 1 2020, 12:53 AM
gchatelet added subscribers: dmgreen, samparker.
gchatelet added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
16150

Ha I see thx for the explanation and good catch!

@dmgreen @samparker what do you think?
I fail to see whether you considered Alignment being 0 when writing D65580.
For context see this line and this line.

As-is, the whole test suite still passes.

gchatelet marked an inline comment as done.Jul 6 2020, 2:51 AM
gchatelet added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
16150

A gentle ping @dmgreen @samparker

@courbet I'm still willing to push this one. Shall I remove [NFC] and call it a day?
AFAICT it's the only show stopper here.