Page MenuHomePhabricator

[amdgpu] Enhance load widening in the constant address space.
Needs RevisionPublic

Authored by hliao on Dec 9 2020, 10:22 PM.

Details

Reviewers
arsenm
rampitec
Summary
  • Support the widening of non DWORD-aligned sub-DWORD loads from the constant space in SelectionDAG.
  • Remove the late codegen preparation pass.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

hliao created this revision.Dec 9 2020, 10:22 PM
hliao requested review of this revision.Dec 9 2020, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 10:22 PM
arsenm added inline comments.Dec 14 2020, 3:08 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7981–7983

PseudoSourceValues can be permissible. However the first situations I think of at the top of my head are all cases where we would have emitted the correct code in the first place

7985–7986

I'm not 100% comfortable relying on the IR value here. Can you just use the DAG known bits?

hliao added inline comments.Dec 22 2020, 9:51 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7981–7983

Yeah, as pseudosource value is generated by the backend, where optimal code should be generated from the beginning. If there are cases introduced later, we may consider to enhance that original code and extend the support here.

7985–7986

Here's the tradeoff I have to make based on the current alias interface in the backend. The alias checking in MI has the assumption that the MachineMemOperand offset should never be any negative offsets. But, the transformation here needs to rebase the pointer by a negative one to be DWORD-aligned. Without breaking the assumption so far, we need to adjust IR mapping to ensure the correctness of alias checking.

arsenm added inline comments.Dec 22 2020, 12:37 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7985–7986

How is that assumed? That sounds broken

arsenm added inline comments.Mar 31 2021, 4:44 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
7985–7986

Needs a comment explaining why this is looking at the IR value. Technically we could just drop the reference (since these are known to be invariant-ish loads, I'm not sure we get much out of the aliasing information)

arsenm requested changes to this revision.Mar 31 2021, 4:44 PM
This revision now requires changes to proceed.Mar 31 2021, 4:44 PM
foad added a subscriber: foad.Apr 1 2021, 2:35 AM