This is an archive of the discontinued LLVM Phabricator instance.

Differential D107305

[GlobalISel] Combine G_ASSERT_*EXT + G_LOAD -> G_S/ZEXTLOAD
AbandonedPublic

Authored by paquette on Aug 2 2021, 1:22 PM.

Details

Reviewers
aemerson
arsenm
Summary

CallLowering can emit hint instructions (G_ASSERT_ZEXT, G_ASSERT_SEXT) to communicate whether or not a parameter/return value is expected to be sign or zero extended.

When we run into one of these along with a G_LOAD, we need to be careful to select the correct type of extended load.

e.g.

https://godbolt.org/z/x3oG4s3MT

In the above example, without this patch, GISel will not emit a sign-extended load. SDAG has the correct behaviour here.

The work here is identical to how we already combine G_SEXT_INREG with G_LOAD. However, because this can be important for ABI-conformance, it needs to run at all optimization levels. So, this is split out into its own combine which runs under combines_for_extload.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.Transforms/LoopUnroll/ARM::multi-blocks.ll
70 msx64 windows > LLVM.Transforms/LoopUnroll/ARM::multi-blocks.ll

Event Timeline

paquette created this revision.Aug 2 2021, 1:22 PM
Herald added subscribers: jfb, hiraditya, rovka. · View Herald TranscriptAug 2 2021, 1:22 PM
paquette requested review of this revision.Aug 2 2021, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2021, 1:22 PM
Herald added a subscriber: wdng. · View Herald Transcript
paquette mentioned this in D96915: [AArch64][GlobalISel] Emit G_ASSERT_SEXT for SExt parameters in CallLowering.Aug 2 2021, 1:23 PM
arsenm added a comment.Aug 2 2021, 1:26 PM

It seems to me like you're working around a correctness issue in a combine. The load is incorrect to begin with, you can't fix it up later like this

aemerson added a comment.Aug 2 2021, 1:34 PM

I don't think this is right. G_ASSERT_SEXT is supposed to only provide information for optimizations about the bits of the source. Using it to justify transforming the source seems the wrong place to fix it. We should be emitting the right kind of load in call lowering.

aemerson added a comment.Aug 2 2021, 1:35 PM
In D107305#2920760, @arsenm wrote:

It seems to me like you're working around a correctness issue in a combine. The load is incorrect to begin with, you can't fix it up later like this

Beat me to it.

Harbormaster completed remote builds in B117509: Diff 363557.Aug 2 2021, 2:07 PM
paquette abandoned this revision.Aug 2 2021, 2:38 PM

Fixing in AArch64CallLowering in D107313

Revision Contents

PathSize
llvm/
include/
llvm/
CodeGen/
GlobalISel/
10 lines
Target/
GlobalISel/
18 lines
lib/
CodeGen/
GlobalISel/
19 lines
test/
CodeGen/
AArch64/
GlobalISel/
153 lines

Diff 363557

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h

Show First 20 Lines • Show All 148 Lines • ▼ Show 20 Linespublic:
bool tryCombineIndexedLoadStore(MachineInstr &MI); bool tryCombineIndexedLoadStore(MachineInstr &MI);
bool matchCombineIndexedLoadStore(MachineInstr &MI, IndexedLoadStoreMatchInfo &MatchInfo); bool matchCombineIndexedLoadStore(MachineInstr &MI, IndexedLoadStoreMatchInfo &MatchInfo);
void applyCombineIndexedLoadStore(MachineInstr &MI, IndexedLoadStoreMatchInfo &MatchInfo); void applyCombineIndexedLoadStore(MachineInstr &MI, IndexedLoadStoreMatchInfo &MatchInfo);
bool matchSextTruncSextLoad(MachineInstr &MI); bool matchSextTruncSextLoad(MachineInstr &MI);
void applySextTruncSextLoad(MachineInstr &MI); void applySextTruncSextLoad(MachineInstr &MI);
/// Match sext_inreg(load p), imm -> sextload p /// Match sext_inreg(load p), imm -> sextload p
bool matchSextInRegOfLoad(MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo); /// assert_sext(load p), imm -> sextload p
void applySextInRegOfLoad(MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo); /// assert_zext(load p), imm -> zextload p
bool
matchExtLoadFromHintOrSextInReg(MachineInstr &MI,
std::tuple<Register, unsigned> &MatchInfo);
void
applyExtLoadFromHintOrSextInReg(MachineInstr &MI,
std::tuple<Register, unsigned> &MatchInfo);
/// Try to combine G_[SU]DIV and G_[SU]REM into a single G_[SU]DIVREM /// Try to combine G_[SU]DIV and G_[SU]REM into a single G_[SU]DIVREM
/// when their source operands are identical. /// when their source operands are identical.
bool matchCombineDivRem(MachineInstr &MI, MachineInstr *&OtherMI); bool matchCombineDivRem(MachineInstr &MI, MachineInstr *&OtherMI);
void applyCombineDivRem(MachineInstr &MI, MachineInstr *&OtherMI); void applyCombineDivRem(MachineInstr &MI, MachineInstr *&OtherMI);
/// If a brcond's true block is not the fallthrough, make it so by inverting /// If a brcond's true block is not the fallthrough, make it so by inverting
/// the condition and swapping operands. /// the condition and swapping operands.
▲ Show 20 Lines • Show All 458 Lines • Show Last 20 Lines

llvm/include/llvm/Target/GlobalISel/Combine.td

Show First 20 Lines • Show All 112 Lines • ▼ Show 20 Lines
class GIApplyKind; class GIApplyKind;
class GIApplyKindWithArgs; class GIApplyKindWithArgs;
def register_matchinfo: GIDefMatchData<"Register">; def register_matchinfo: GIDefMatchData<"Register">;
def int64_matchinfo: GIDefMatchData<"int64_t">; def int64_matchinfo: GIDefMatchData<"int64_t">;
def apint_matchinfo : GIDefMatchData<"APInt">; def apint_matchinfo : GIDefMatchData<"APInt">;
def build_fn_matchinfo : def build_fn_matchinfo :
GIDefMatchData<"std::function<void(MachineIRBuilder &)>">; GIDefMatchData<"std::function<void(MachineIRBuilder &)>">;
def register_and_unsigned_matchinfo: GIDefMatchData<"std::tuple<Register, unsigned>">;
def copy_prop : GICombineRule< def copy_prop : GICombineRule<
(defs root:$d), (defs root:$d),
(match (COPY $d, $s):$mi, (match (COPY $d, $s):$mi,
[{ return Helper.matchCombineCopy(*${mi}); }]), [{ return Helper.matchCombineCopy(*${mi}); }]),
(apply [{ Helper.applyCombineCopy(*${mi}); }])>; (apply [{ Helper.applyCombineCopy(*${mi}); }])>;
// Same combine as sext_inreg_of_load, but for hint instructions. This should
// run at all opt levels.
def hint_to_extload : GICombineRule<
(defs root:$root, register_and_unsigned_matchinfo:$matchinfo),
(match (wip_match_opcode G_ASSERT_SEXT, G_ASSERT_ZEXT):$root,
[{ return Helper.matchExtLoadFromHintOrSextInReg(*${root}, ${matchinfo}); }]),
(apply [{ Helper.applyExtLoadFromHintOrSextInReg(*${root}, ${matchinfo}); }])>;
def extending_loads : GICombineRule< def extending_loads : GICombineRule<
(defs root:$root, extending_load_matchdata:$matchinfo), (defs root:$root, extending_load_matchdata:$matchinfo),
(match (wip_match_opcode G_LOAD, G_SEXTLOAD, G_ZEXTLOAD):$root, (match (wip_match_opcode G_LOAD, G_SEXTLOAD, G_ZEXTLOAD):$root,
[{ return Helper.matchCombineExtendingLoads(*${root}, ${matchinfo}); }]), [{ return Helper.matchCombineExtendingLoads(*${root}, ${matchinfo}); }]),
(apply [{ Helper.applyCombineExtendingLoads(*${root}, ${matchinfo}); }])>; (apply [{ Helper.applyCombineExtendingLoads(*${root}, ${matchinfo}); }])>;
def combines_for_extload: GICombineGroup<[extending_loads]>; def combines_for_extload: GICombineGroup<[extending_loads, hint_to_extload]>;
def sext_trunc_sextload : GICombineRule< def sext_trunc_sextload : GICombineRule<
(defs root:$d), (defs root:$d),
(match (wip_match_opcode G_SEXT_INREG):$d, (match (wip_match_opcode G_SEXT_INREG):$d,
[{ return Helper.matchSextTruncSextLoad(*${d}); }]), [{ return Helper.matchSextTruncSextLoad(*${d}); }]),
(apply [{ Helper.applySextTruncSextLoad(*${d}); }])>; (apply [{ Helper.applySextTruncSextLoad(*${d}); }])>;
def sext_inreg_of_load_matchdata : GIDefMatchData<"std::tuple<Register, unsigned>">;
def sext_inreg_of_load : GICombineRule< def sext_inreg_of_load : GICombineRule<
(defs root:$root, sext_inreg_of_load_matchdata:$matchinfo), (defs root:$root, register_and_unsigned_matchinfo:$matchinfo),
(match (wip_match_opcode G_SEXT_INREG):$root, (match (wip_match_opcode G_SEXT_INREG):$root,
[{ return Helper.matchSextInRegOfLoad(*${root}, ${matchinfo}); }]), [{ return Helper.matchExtLoadFromHintOrSextInReg(*${root}, ${matchinfo}); }]),
(apply [{ Helper.applySextInRegOfLoad(*${root}, ${matchinfo}); }])>; (apply [{ Helper.applyExtLoadFromHintOrSextInReg(*${root}, ${matchinfo}); }])>;
def combine_indexed_load_store : GICombineRule< def combine_indexed_load_store : GICombineRule<
(defs root:$root, indexed_load_store_matchdata:$matchinfo), (defs root:$root, indexed_load_store_matchdata:$matchinfo),
(match (wip_match_opcode G_LOAD, G_SEXTLOAD, G_ZEXTLOAD, G_STORE):$root, (match (wip_match_opcode G_LOAD, G_SEXTLOAD, G_ZEXTLOAD, G_STORE):$root,
[{ return Helper.matchCombineIndexedLoadStore(*${root}, ${matchinfo}); }]), [{ return Helper.matchCombineIndexedLoadStore(*${root}, ${matchinfo}); }]),
(apply [{ Helper.applyCombineIndexedLoadStore(*${root}, ${matchinfo}); }])>; (apply [{ Helper.applyCombineIndexedLoadStore(*${root}, ${matchinfo}); }])>;
def opt_brcond_by_inverting_cond_matchdata : GIDefMatchData<"MachineInstr *">; def opt_brcond_by_inverting_cond_matchdata : GIDefMatchData<"MachineInstr *">;
▲ Show 20 Lines • Show All 570 Lines • Show Last 20 Lines

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

Show First 20 Lines • Show All 678 Lines • ▼ Show 20 Lines
void CombinerHelper::applySextTruncSextLoad(MachineInstr &MI) { void CombinerHelper::applySextTruncSextLoad(MachineInstr &MI) {
assert(MI.getOpcode() == TargetOpcode::G_SEXT_INREG); assert(MI.getOpcode() == TargetOpcode::G_SEXT_INREG);
Builder.setInstrAndDebugLoc(MI); Builder.setInstrAndDebugLoc(MI);
Builder.buildCopy(MI.getOperand(0).getReg(), MI.getOperand(1).getReg()); Builder.buildCopy(MI.getOperand(0).getReg(), MI.getOperand(1).getReg());
MI.eraseFromParent(); MI.eraseFromParent();
} }
bool CombinerHelper::matchSextInRegOfLoad( bool CombinerHelper::matchExtLoadFromHintOrSextInReg(
MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo) { MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo) {
assert(MI.getOpcode() == TargetOpcode::G_SEXT_INREG); unsigned Opc = MI.getOpcode();
assert(Opc == TargetOpcode::G_SEXT_INREG ||
Opc == TargetOpcode::G_ASSERT_SEXT ||
Opc == TargetOpcode::G_ASSERT_ZEXT);
// Only supports scalars for now. // Only supports scalars for now.
if (MRI.getType(MI.getOperand(0).getReg()).isVector()) if (MRI.getType(MI.getOperand(0).getReg()).isVector())
return false; return false;
Register SrcReg = MI.getOperand(1).getReg(); Register SrcReg = MI.getOperand(1).getReg();
auto *LoadDef = getOpcodeDef<GLoad>(SrcReg, MRI); auto *LoadDef = getOpcodeDef<GLoad>(SrcReg, MRI);
if (!LoadDef || !MRI.hasOneNonDBGUse(LoadDef->getOperand(0).getReg()) || if (!LoadDef || !MRI.hasOneNonDBGUse(LoadDef->getOperand(0).getReg()) ||
Show All 12 Linesbool CombinerHelper::matchExtLoadFromHintOrSextInReg(
// Don't bother creating a non-power-2 sextload, it will likely be broken up // Don't bother creating a non-power-2 sextload, it will likely be broken up
// anyway for most targets. // anyway for most targets.
if (!isPowerOf2_32(NewSizeBits)) if (!isPowerOf2_32(NewSizeBits))
return false; return false;
MatchInfo = std::make_tuple(LoadDef->getDstReg(), NewSizeBits); MatchInfo = std::make_tuple(LoadDef->getDstReg(), NewSizeBits);
return true; return true;
} }
void CombinerHelper::applySextInRegOfLoad( void CombinerHelper::applyExtLoadFromHintOrSextInReg(
MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo) { MachineInstr &MI, std::tuple<Register, unsigned> &MatchInfo) {
assert(MI.getOpcode() == TargetOpcode::G_SEXT_INREG); unsigned Opc = MI.getOpcode();
assert(Opc == TargetOpcode::G_SEXT_INREG ||
Opc == TargetOpcode::G_ASSERT_SEXT ||
Opc == TargetOpcode::G_ASSERT_ZEXT);
Register LoadReg; Register LoadReg;
unsigned ScalarSizeBits; unsigned ScalarSizeBits;
std::tie(LoadReg, ScalarSizeBits) = MatchInfo; std::tie(LoadReg, ScalarSizeBits) = MatchInfo;
GLoad *LoadDef = cast<GLoad>(MRI.getVRegDef(LoadReg)); GLoad *LoadDef = cast<GLoad>(MRI.getVRegDef(LoadReg));
// If we have the following: // If we have the following:
// %ld = G_LOAD %ptr, (load 2) // %ld = G_LOAD %ptr, (load 2)
// %ext = G_SEXT_INREG %ld, 8 // %ext = G_SEXT_INREG %ld, 8
// ==> // ==>
// %ld = G_SEXTLOAD %ptr (load 1) // %ld = G_SEXTLOAD %ptr (load 1)
auto &MMO = LoadDef->getMMO(); auto &MMO = LoadDef->getMMO();
Builder.setInstrAndDebugLoc(*LoadDef); Builder.setInstrAndDebugLoc(*LoadDef);
auto &MF = Builder.getMF(); auto &MF = Builder.getMF();
auto PtrInfo = MMO.getPointerInfo(); auto PtrInfo = MMO.getPointerInfo();
auto *NewMMO = MF.getMachineMemOperand(&MMO, PtrInfo, ScalarSizeBits / 8); auto *NewMMO = MF.getMachineMemOperand(&MMO, PtrInfo, ScalarSizeBits / 8);
Builder.buildLoadInstr(TargetOpcode::G_SEXTLOAD, MI.getOperand(0).getReg(), unsigned LoadOpc = Opc == TargetOpcode::G_ASSERT_ZEXT
? TargetOpcode::G_ZEXTLOAD
: TargetOpcode::G_SEXTLOAD;
Builder.buildLoadInstr(LoadOpc, MI.getOperand(0).getReg(),
LoadDef->getPointerReg(), *NewMMO); LoadDef->getPointerReg(), *NewMMO);
MI.eraseFromParent(); MI.eraseFromParent();
} }
bool CombinerHelper::findPostIndexCandidate(MachineInstr &MI, Register &Addr, bool CombinerHelper::findPostIndexCandidate(MachineInstr &MI, Register &Addr,
Register &Base, Register &Offset) { Register &Base, Register &Offset) {
auto &MF = *MI.getParent()->getParent(); auto &MF = *MI.getParent()->getParent();
const auto &TLI = *MF.getSubtarget().getTargetLowering(); const auto &TLI = *MF.getSubtarget().getTargetLowering();
▲ Show 20 Lines • Show All 3,528 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/combine-hint-extload.mir

  • This file was added.
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple aarch64 -debugify-and-strip-all-safe -O0 -run-pass=aarch64-prelegalizer-combiner --aarch64prelegalizercombinerhelper-only-enable-rule="hint_to_extload" -global-isel -verify-machineinstrs %s -o - | FileCheck %s
# REQUIRES: asserts
...
---
name: assert_sext_sextload
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: assert_sext_sextload
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %hint:_(s32) = G_SEXTLOAD %ptr(p0) :: (load (s8))
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
%hint:_(s32) = G_ASSERT_SEXT %load, 8
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: assert_zext_zextload
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: assert_zext_zextload
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %hint:_(s32) = G_ZEXTLOAD %ptr(p0) :: (load (s8))
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
%hint:_(s32) = G_ASSERT_ZEXT %load, 8
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: assert_sext_sextload_hint_smaller
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: assert_sext_sextload_hint_smaller
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %hint:_(s32) = G_SEXTLOAD %ptr(p0) :: (load (s8), align 2)
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s16))
%hint:_(s32) = G_ASSERT_SEXT %load, 8
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: assert_sext_sextload_memop_smaller
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: assert_sext_sextload_memop_smaller
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %hint:_(s32) = G_SEXTLOAD %ptr(p0) :: (load (s8))
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
%hint:_(s32) = G_ASSERT_SEXT %load, 16
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: hint_imm_too_small
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: hint_imm_too_small
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
; CHECK: %hint:_(s32) = G_ASSERT_SEXT %load, 4
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
%hint:_(s32) = G_ASSERT_SEXT %load, 4
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: hint_imm_non_pow_2
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: hint_imm_non_pow_2
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %load:_(s32) = G_LOAD %ptr(p0) :: (load (s16))
; CHECK: %hint:_(s32) = G_ASSERT_SEXT %load, 9
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s16))
%hint:_(s32) = G_ASSERT_SEXT %load, 9
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0
...
---
name: more_than_one_use
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: more_than_one_use
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
; CHECK: %hint:_(s32) = G_ASSERT_SEXT %load, 8
; CHECK: %add:_(s32) = G_ADD %hint, %load
; CHECK: $w0 = COPY %add(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load (s8))
%hint:_(s32) = G_ASSERT_SEXT %load, 8
%add:_(s32) = G_ADD %hint, %load
$w0 = COPY %add(s32)
RET_ReallyLR implicit $w0
...
---
name: not_simple
tracksRegLiveness: true
body: |
bb.0:
liveins: $x0
; CHECK-LABEL: name: not_simple
; CHECK: liveins: $x0
; CHECK: %ptr:_(p0) = COPY $x0
; CHECK: %load:_(s32) = G_LOAD %ptr(p0) :: (load seq_cst (s8))
; CHECK: %hint:_(s32) = G_ASSERT_SEXT %load, 8
; CHECK: $w0 = COPY %hint(s32)
; CHECK: RET_ReallyLR implicit $w0
%ptr:_(p0) = COPY $x0
%load:_(s32) = G_LOAD %ptr(p0) :: (load seq_cst (s8))
%hint:_(s32) = G_ASSERT_SEXT %load, 8
$w0 = COPY %hint(s32)
RET_ReallyLR implicit $w0