Skip to content

Commit

Permalink
[llvm][AArch64] Handle arrays of struct properly (from IR)
Browse files Browse the repository at this point in the history
This only applies to FastIsel. GlobalIsel seems to sidestep
the issue.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46996

One of the things we do in llvm is decide if a type needs
consecutive registers. Previously, we just checked if it
was an array or not.
(plus an SVE specific check that is not changing here)

This causes some confusion when you arbitrary IR like:
```
%T1 = type { double, i1 };
define [ 1 x %T1 ] @foo() {
entry:
  ret [ 1 x %T1 ] zeroinitializer
}
```

We see it is an array so we call CC_AArch64_Custom_Block
which bails out when it sees the i1, a type we don't want
to put into a block.

This leaves the location of the double in some kind of
intermediate state and leads to odd codegen. Which then crashes
the backend because it doesn't know how to implement
what it's been asked for.

You get this:
```
  renamable $d0 = FMOVD0
  $w0 = COPY killed renamable $d0
```

Rather than this:
```
  $d0 = FMOVD0
  $w0 = COPY $wzr
```

The backend knows how to copy 64 bit to 64 bit registers,
but not 64 to 32. It can certainly be taught how but the real
issue seems to be us even trying to assign a register block
in the first place.

This change makes the logic of
AArch64TargetLowering::functionArgumentNeedsConsecutiveRegisters
a bit more in depth. If we find an array, also check that all the
nested aggregates in that array have a single member type.

Then CC_AArch64_Custom_Block's assumption of a type that looks
like [ N x type ] will be valid and we get the expected codegen.

New tests have been added to exercise these situations. Note that
some of the output is not ABI compliant. The aim of this change is
to simply handle these situations and not to make our processing
of arbitrary IR ABI compliant.

Reviewed By: efriedma

Differential Revision: https://reviews.llvm.org/D104123
  • Loading branch information
DavidSpickett committed Jun 16, 2021
1 parent 87784cc commit e4ecd83
Show file tree
Hide file tree
Showing 10 changed files with 540 additions and 21 deletions.
3 changes: 2 additions & 1 deletion llvm/include/llvm/CodeGen/TargetLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -4001,7 +4001,8 @@ class TargetLowering : public TargetLoweringBase {
/// must be passed in a block of consecutive registers.
virtual bool
functionArgumentNeedsConsecutiveRegisters(Type *Ty, CallingConv::ID CallConv,
bool isVarArg) const {
bool isVarArg,
const DataLayout &DL) const {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ void CallLowering::splitToValueTypes(const ArgInfo &OrigArg,
assert(OrigArg.Regs.size() == SplitVTs.size() && "Regs / types mismatch");

bool NeedsRegBlock = TLI->functionArgumentNeedsConsecutiveRegisters(
OrigArg.Ty, CallConv, false);
OrigArg.Ty, CallConv, false, DL);
for (unsigned i = 0, e = SplitVTs.size(); i < e; ++i) {
Type *SplitTy = SplitVTs[i].getTypeForEVT(Ctx);
SplitArgs.emplace_back(OrigArg.Regs[i], SplitTy, OrigArg.Flags[0],
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ bool FastISel::lowerCallTo(CallLoweringInfo &CLI) {
if (Arg.IsByVal)
FinalType = cast<PointerType>(Arg.Ty)->getElementType();
bool NeedsRegBlock = TLI.functionArgumentNeedsConsecutiveRegisters(
FinalType, CLI.CallConv, CLI.IsVarArg);
FinalType, CLI.CallConv, CLI.IsVarArg, DL);

ISD::ArgFlagsTy Flags;
if (Arg.IsZExt)
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1933,7 +1933,7 @@ void SelectionDAGBuilder::visitRet(const ReturnInst &I) {

bool NeedsRegBlock = TLI.functionArgumentNeedsConsecutiveRegisters(
I.getOperand(0)->getType(), F->getCallingConv(),
/*IsVarArg*/ false);
/*IsVarArg*/ false, DL);

ISD::NodeType ExtendKind = ISD::ANY_EXTEND;
if (F->getAttributes().hasAttribute(AttributeList::ReturnIndex,
Expand Down Expand Up @@ -9432,7 +9432,7 @@ TargetLowering::LowerCallTo(TargetLowering::CallLoweringInfo &CLI) const {
CLI.IsTailCall = false;
} else {
bool NeedsRegBlock = functionArgumentNeedsConsecutiveRegisters(
CLI.RetTy, CLI.CallConv, CLI.IsVarArg);
CLI.RetTy, CLI.CallConv, CLI.IsVarArg, DL);
for (unsigned I = 0, E = RetTys.size(); I != E; ++I) {
ISD::ArgFlagsTy Flags;
if (NeedsRegBlock) {
Expand Down Expand Up @@ -9492,7 +9492,7 @@ TargetLowering::LowerCallTo(TargetLowering::CallLoweringInfo &CLI) const {
if (Args[i].IsByVal)
FinalType = cast<PointerType>(Args[i].Ty)->getElementType();
bool NeedsRegBlock = functionArgumentNeedsConsecutiveRegisters(
FinalType, CLI.CallConv, CLI.IsVarArg);
FinalType, CLI.CallConv, CLI.IsVarArg, DL);
for (unsigned Value = 0, NumValues = ValueVTs.size(); Value != NumValues;
++Value) {
EVT VT = ValueVTs[Value];
Expand Down Expand Up @@ -10043,7 +10043,7 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
if (Arg.hasAttribute(Attribute::ByVal))
FinalType = Arg.getParamByValType();
bool NeedsRegBlock = TLI->functionArgumentNeedsConsecutiveRegisters(
FinalType, F.getCallingConv(), F.isVarArg());
FinalType, F.getCallingConv(), F.isVarArg(), DL);
for (unsigned Value = 0, NumValues = ValueVTs.size();
Value != NumValues; ++Value) {
EVT VT = ValueVTs[Value];
Expand Down
19 changes: 11 additions & 8 deletions llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "llvm/ADT/Twine.h"
#include "llvm/Analysis/ObjCARCUtil.h"
#include "llvm/Analysis/VectorUtils.h"
#include "llvm/CodeGen/Analysis.h"
#include "llvm/CodeGen/CallingConvLower.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
Expand Down Expand Up @@ -17210,15 +17211,17 @@ Value *AArch64TargetLowering::emitStoreConditional(IRBuilderBase &Builder,
}

bool AArch64TargetLowering::functionArgumentNeedsConsecutiveRegisters(
Type *Ty, CallingConv::ID CallConv, bool isVarArg) const {
if (Ty->isArrayTy())
return true;

const TypeSize &TySize = Ty->getPrimitiveSizeInBits();
if (TySize.isScalable() && TySize.getKnownMinSize() > 128)
return true;
Type *Ty, CallingConv::ID CallConv, bool isVarArg,
const DataLayout &DL) const {
if (!Ty->isArrayTy()) {
const TypeSize &TySize = Ty->getPrimitiveSizeInBits();
return TySize.isScalable() && TySize.getKnownMinSize() > 128;
}

return false;
// All non aggregate members of the type must have the same type
SmallVector<EVT> ValueVTs;
ComputeValueVTs(*this, DL, Ty, ValueVTs);
return is_splat(ValueVTs);
}

bool AArch64TargetLowering::shouldNormalizeToSelectSequence(LLVMContext &,
Expand Down
7 changes: 4 additions & 3 deletions llvm/lib/Target/AArch64/AArch64ISelLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,10 @@ class AArch64TargetLowering : public TargetLowering {
MachineMemOperand::Flags getTargetMMOFlags(
const Instruction &I) const override;

bool functionArgumentNeedsConsecutiveRegisters(Type *Ty,
CallingConv::ID CallConv,
bool isVarArg) const override;
bool functionArgumentNeedsConsecutiveRegisters(
Type *Ty, CallingConv::ID CallConv, bool isVarArg,
const DataLayout &DL) const override;

/// Used for exception handling on Win64.
bool needsFixedCatchObjects() const override;

Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/ARM/ARMISelLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20131,7 +20131,8 @@ Align ARMTargetLowering::getABIAlignmentForCallingConv(
/// [N x i32] or [N x i64]. This allows front-ends to skip emitting padding when
/// passing according to AAPCS rules.
bool ARMTargetLowering::functionArgumentNeedsConsecutiveRegisters(
Type *Ty, CallingConv::ID CallConv, bool isVarArg) const {
Type *Ty, CallingConv::ID CallConv, bool isVarArg,
const DataLayout &DL) const {
if (getEffectiveCallingConv(CallConv, isVarArg) !=
CallingConv::ARM_AAPCS_VFP)
return false;
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/ARM/ARMISelLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,8 @@ class VectorType;
/// Returns true if an argument of type Ty needs to be passed in a
/// contiguous block of registers in calling convention CallConv.
bool functionArgumentNeedsConsecutiveRegisters(
Type *Ty, CallingConv::ID CallConv, bool isVarArg) const override;
Type *Ty, CallingConv::ID CallConv, bool isVarArg,
const DataLayout &DL) const override;

/// If a physical register, this returns the register that receives the
/// exception address on entry to an EH pad.
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/PowerPC/PPCISelLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,8 @@ namespace llvm {
/// Returns true if an argument of type Ty needs to be passed in a
/// contiguous block of registers in calling convention CallConv.
bool functionArgumentNeedsConsecutiveRegisters(
Type *Ty, CallingConv::ID CallConv, bool isVarArg) const override {
Type *Ty, CallingConv::ID CallConv, bool isVarArg,
const DataLayout &DL) const override {
// We support any array type as "consecutive" block in the parameter
// save area. The element type defines the alignment requirement and
// whether the argument should go in GPRs, FPRs, or VRs if available.
Expand Down
Loading

0 comments on commit e4ecd83

Please sign in to comment.