This is an archive of the discontinued LLVM Phabricator instance.

Propagate Dst and Src alignment for mem{cpy|move}
Needs ReviewPublic

Authored by gchatelet on Sep 23 2022, 7:55 AM.

Details

Reviewers
courbet

Event Timeline

gchatelet created this revision.Sep 23 2022, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2022, 7:55 AM
gchatelet requested review of this revision.Sep 23 2022, 7:55 AM

What about keeping the changes completely mechanical and semantically the same for this large patch and fixing the issues separately ?

llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
98

Why this change ? I can see that this does not read, but what about keeping this change completely mechanical ?

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
3268

Looks like this might have been incorrect. Maybe add the author to that patch as an FYI ?

llvm/lib/Target/ARM/ARMSelectionDAGInfo.cpp
188

this used to be: if (max(DstAlign, SrcAlign) < 4), ie.g. if (DstAlign < Align(4) && SrcAlign < Align(4))

llvm/lib/Target/BPF/BPFSelectionDAGInfo.cpp
31

This is not NFC.

llvm/lib/Target/Hexagon/HexagonSelectionDAGInfo.cpp
25

ditto

llvm/lib/Target/X86/X86SelectionDAGInfo.cpp
186

Can you do that in a separate patch ?

llvm/lib/Target/XCore/XCoreISelLowering.cpp
1797

This might have been a bug, but again, let's fix it separately.

llvm/lib/Target/XCore/XCoreSelectionDAGInfo.cpp
25

this should be ( || )

arichardson added inline comments.Sep 25 2022, 10:10 AM
llvm/include/llvm/CodeGen/SelectionDAG.h
1062

This function has so many arguments (and I actually added another one downstream in the CHERI fork), would it make sense to introduce a struct for some of them (similar to struct MemOp` in TargetLowering?
If one struct for all the copy operands does not sound like a good idea to you, using one that holds SdValue+Align+MachinePointerInfo could also reduce the number of arguments.