This is an archive of the discontinued LLVM Phabricator instance.

Fix alignment in AArch64InstructionSelector::emitConstantPoolEntry()
ClosedPublic

Authored by hans on Apr 25 2019, 3:09 AM.

Download Raw Diff

Details

Reviewers

aemerson
t.p.northover

Commits

rG5d5ee4aff748: Fix alignment in AArch64InstructionSelector::emitConstantPoolEntry()
rL359287: Fix alignment in AArch64InstructionSelector::emitConstantPoolEntry()

Summary

The code was using the alignment of a pointer to the constant, not the
alignment of the constant itself.

I guess we got away with it so far because the pointer alignment is
fairly high, but we did end up under-aligning <16 x i8> vectors,
which was caught in the Chromium build (crbug.com/953815).

Please take a look.
+Amara who added the code in r354616, and Tim for general AArch64 goodness maybe in a closer timezone.

Diff Detail

Event Timeline

hans created this revision.Apr 25 2019, 3:09 AM

Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 3:09 AM

Herald added subscribers: hiraditya, kristof.beyls, javed.absar. · View Herald Transcript

MaskRay added a subscriber: MaskRay.Apr 25 2019, 6:02 AM

I'll commit this now since it seems like an obvious fix. Happy to take post-commit review comments.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 26 2019, 1:29 AM

Closed by commit rL359287: Fix alignment in AArch64InstructionSelector::emitConstantPoolEntry() (authored by hans). · Explain Why

This revision was automatically updated to reflect the committed changes.

Yep. Very simple.

Hi Hans,

Thanks for doing this. Sorry I’m on vacation at the moment so didn’t see it.

Cheers,
Amara

Revision Contents

Path

Size

llvm/

lib/

Target/

AArch64/

AArch64InstructionSelector.cpp

2 lines

test/

CodeGen/

AArch64/

GlobalISel/

select-shuffle-vector.mir

6 lines

Diff 196600

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp

Show First 20 Lines • Show All 2,566 Lines • ▼ Show 20 Lines	if (ScalarDef->getOpcode() != TargetOpcode::G_CONSTANT) {
Idxs.push_back(ScalarDef->getOperand(1).getCImm()->getSExtValue());		Idxs.push_back(ScalarDef->getOperand(1).getCImm()->getSExtValue());
}		}
}		}
}		}

unsigned		unsigned
AArch64InstructionSelector::emitConstantPoolEntry(Constant *CPVal,		AArch64InstructionSelector::emitConstantPoolEntry(Constant *CPVal,
MachineFunction &MF) const {		MachineFunction &MF) const {
Type *CPTy = CPVal->getType()->getPointerTo();		Type *CPTy = CPVal->getType();
unsigned Align = MF.getDataLayout().getPrefTypeAlignment(CPTy);		unsigned Align = MF.getDataLayout().getPrefTypeAlignment(CPTy);
if (Align == 0)		if (Align == 0)
Align = MF.getDataLayout().getTypeAllocSize(CPTy);		Align = MF.getDataLayout().getTypeAllocSize(CPTy);

MachineConstantPool *MCP = MF.getConstantPool();		MachineConstantPool *MCP = MF.getConstantPool();
return MCP->getConstantPoolIndex(CPVal, Align);		return MCP->getConstantPoolIndex(CPVal, Align);
}		}

▲ Show 20 Lines • Show All 730 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/select-shuffle-vector.mir

	Show First 20 Lines • Show All 76 Lines • ▼ Show 20 Lines
	tracksRegLiveness: true			tracksRegLiveness: true
	body: \|			body: \|
	bb.1 (%ir-block.0):			bb.1 (%ir-block.0):
	liveins: $q0, $q1			liveins: $q0, $q1

	; CHECK-LABEL: name: shuffle_v4i32			; CHECK-LABEL: name: shuffle_v4i32
	; CHECK: constants:			; CHECK: constants:
	; CHECK: value: '<16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 12, i8 13, i8 14, i8 15, i8 0, i8 1, i8 2, i8 3>'			; CHECK: value: '<16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 12, i8 13, i8 14, i8 15, i8 0, i8 1, i8 2, i8 3>'
	; CHECK: alignment: 8			; CHECK: alignment: 16
	; CHECK: isTargetSpecific: false			; CHECK: isTargetSpecific: false
	; CHECK: liveins: $q0, $q1			; CHECK: liveins: $q0, $q1
	; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0			; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0
	; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1			; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1
	; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0			; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0
	; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0			; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0
	; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1			; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1
	; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]			; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]
	Show All 18 Lines
	tracksRegLiveness: true			tracksRegLiveness: true
	body: \|			body: \|
	bb.1 (%ir-block.0):			bb.1 (%ir-block.0):
	liveins: $q0, $q1			liveins: $q0, $q1

	; CHECK-LABEL: name: shuffle_tbl_v4i32			; CHECK-LABEL: name: shuffle_tbl_v4i32
	; CHECK: constants:			; CHECK: constants:
	; CHECK: value: '<16 x i8> <i8 20, i8 21, i8 22, i8 23, i8 28, i8 29, i8 30, i8 31, i8 4, i8 5, i8 6, i8 7, i8 0, i8 1, i8 2, i8 3>'			; CHECK: value: '<16 x i8> <i8 20, i8 21, i8 22, i8 23, i8 28, i8 29, i8 30, i8 31, i8 4, i8 5, i8 6, i8 7, i8 0, i8 1, i8 2, i8 3>'
	; CHECK: alignment: 8			; CHECK: alignment: 16
	; CHECK: isTargetSpecific: false			; CHECK: isTargetSpecific: false
	; CHECK: liveins: $q0, $q1			; CHECK: liveins: $q0, $q1
	; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0			; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0
	; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1			; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1
	; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0			; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0
	; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0			; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0
	; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1			; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1
	; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]			; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]
	Show All 19 Lines
	tracksRegLiveness: true			tracksRegLiveness: true
	body: \|			body: \|
	bb.1 (%ir-block.0):			bb.1 (%ir-block.0):
	liveins: $q0, $q1			liveins: $q0, $q1

	; CHECK-LABEL: name: shuffle_v2i64			; CHECK-LABEL: name: shuffle_v2i64
	; CHECK: constants:			; CHECK: constants:
	; CHECK: value: '<16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7>'			; CHECK: value: '<16 x i8> <i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7, i8 0, i8 1, i8 2, i8 3, i8 4, i8 5, i8 6, i8 7>'
	; CHECK: alignment: 8			; CHECK: alignment: 16
	; CHECK: isTargetSpecific: false			; CHECK: isTargetSpecific: false
	; CHECK: liveins: $q0, $q1			; CHECK: liveins: $q0, $q1
	; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0			; CHECK: [[COPY:%[0-9]+]]:fpr128 = COPY $q0
	; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1			; CHECK: [[COPY1:%[0-9]+]]:fpr128 = COPY $q1
	; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0			; CHECK: [[ADRP:%[0-9]+]]:gpr64common = ADRP target-flags(aarch64-page) %const.0
	; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0			; CHECK: [[LDRQui:%[0-9]+]]:fpr128 = LDRQui [[ADRP]], target-flags(aarch64-pageoff, aarch64-nc) %const.0
	; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1			; CHECK: [[REG_SEQUENCE:%[0-9]+]]:qq = REG_SEQUENCE [[COPY]], %subreg.qsub0, [[COPY1]], %subreg.qsub1
	; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]			; CHECK: [[TBLv16i8Two:%[0-9]+]]:fpr128 = TBLv16i8Two [[REG_SEQUENCE]], [[LDRQui]]
	Show All 11 Lines