Page MenuHomePhabricator
Differential D130692 Diff 448277 llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

Changeset View

Standalone View

View Options

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 17,643 Lines • ▼ Show 20 Lines if (auto Shift = DAG.getSplatValue(ShiftOp))
Add.getOperand(0), ShiftOp); Add.getOperand(0), ShiftOp);
return true; return true;
} }
} }
return false; return false;
} }
bool tryNarrowZExtGatherIndex(const SDNode *N, SDValue &Index,
paulwalker-armUnsubmitted
Not Done
ReplyInline Actions

I'll reserve judgement but it doesn't really seem worth breaking thus out into a separate function.

You do need to ensure N is treating Index as unsigned before you can shrink the extend. MaskedGatherScatterSDNode has a function to query this.

paulwalker-arm: I'll reserve judgement but it doesn't really seem worth breaking thus out into a separate…
SelectionDAG &DAG) {
SDValue IndexOp0 = Index.getOperand(0);
if (IndexOp0.getValueType().getVectorElementType().getFixedSizeInBits() >=
peterwaller-armUnsubmitted
Not Done
ReplyInline Actions

s/.getVectorElementType().getFixedSizeInBits()/.getScalarSizeInBits()/

peterwaller-arm: s/.getVectorElementType().getFixedSizeInBits()/.getScalarSizeInBits()/
32) {
return false;
}
Index = DAG.getNode(ISD::ZERO_EXTEND, SDLoc(N),
IndexOp0.getValueType().changeVectorElementType(MVT::i32),
IndexOp0);
return true;
}
// Analyse the specified address returning true if a more optimal addressing // Analyse the specified address returning true if a more optimal addressing
// mode is available. When returning true all parameters are updated to reflect // mode is available. When returning true all parameters are updated to reflect
// their recommended values. // their recommended values.
static bool findMoreOptimalIndexType(const MaskedGatherScatterSDNode *N, static bool findMoreOptimalIndexType(const MaskedGatherScatterSDNode *N,
SDValue &BasePtr, SDValue &Index, SDValue &BasePtr, SDValue &Index,
SelectionDAG &DAG) { SelectionDAG &DAG) {
if (Index.getOpcode() == ISD::ZERO_EXTEND) {
if (tryNarrowZExtGatherIndex(N, Index, DAG))
DavidTrubyUnsubmitted
Not Done
ReplyInline Actions

Could you just have the following and remove one level of nesting?

if (Index.getOpcode() == ISD::ZERO_EXTEND && tryNarrowZExtGatherIndex(N, Index, DAG))
  return true;
DavidTruby: Could you just have the following and remove one level of nesting? ``` if (Index.getOpcode()…
return true;
}
// Try to iteratively fold parts of the index into the base pointer to // Try to iteratively fold parts of the index into the base pointer to
// simplify the index as much as possible. // simplify the index as much as possible.
bool Changed = false; bool Changed = false;
while (foldIndexIntoBase(BasePtr, Index, N->getScale(), SDLoc(N), DAG)) while (foldIndexIntoBase(BasePtr, Index, N->getScale(), SDLoc(N), DAG))
Changed = true; Changed = true;
// Only consider element types that are pointer sized as smaller types can // Only consider element types that are pointer sized as smaller types can
// be easily promoted. // be easily promoted.
EVT IndexVT = Index.getValueType(); EVT IndexVT = Index.getValueType();
if (IndexVT.getVectorElementType() != MVT::i64 || IndexVT == MVT::nxv2i64) if (IndexVT.getVectorElementType() != MVT::i64 || IndexVT == MVT::nxv2i64)
return Changed; return Changed;
// Match: // Match:
// Index = step(const) // Index = step(const)
paulwalker-armUnsubmitted
Not Done
ReplyInline Actions

Please pull this out into a separate check before this block, along with a suitable comment. The "fix" is not really related to isVectorShrinkable, it is just that today that is the only logic applicable for fixed length vectors. However, this might change in the future hence why I prefer an isolated check.

paulwalker-arm: Please pull this out into a separate check before this block, along with a suitable comment.
MattDevereauAuthorUnsubmitted
Done
ReplyInline Actions

I'm not sure if I'm missing something obvious here but I don't understand what you're expecting this to look like when this check is seperated. The way this function's logic is structured means it's difficult to bail out cleanly from individual checks here, as we want to fall through this if block instead of returning false. I could use a nested if block, e.g.

//comment
if (!(DataVT.getScalarSizeInBits() == 64 && DataVT.isFixedLengthVector())){
  if (!ISD::isVectorShrinkable... ){
    ...
  }
}

To sort of separate the "temporary" condition, but I believe it still needs to be and logic for the correct behaviour here
Could you please elaborate on what you're expecting?

MattDevereau: I'm not sure if I'm missing something obvious here but I don't understand what you're expecting…
paulwalker-armUnsubmitted
Not Done
ReplyInline Actions

I was thinking of:

// Fixed length vectors are always "packed" so there's no value in the index having a smaller element type than the data.
if (DataVT.isFixedLengthVector() && DataVT.getScalarSizeInBits() == 64)
  return Changed;

It is my assertion that we don't want to fall through because all code after this point is trying to rewrite Index to be a vector of i32s, which is never going to be good for fixed length vector types because Index will just get re-extended during operation legalisation.

paulwalker-arm: I was thinking of: ``` // Fixed length vectors are always "packed" so there's no value in the…
int64_t Stride = 0; int64_t Stride = 0;
if (Index.getOpcode() == ISD::STEP_VECTOR) if (Index.getOpcode() == ISD::STEP_VECTOR)
Stride = cast<ConstantSDNode>(Index.getOperand(0))->getSExtValue(); Stride = cast<ConstantSDNode>(Index.getOperand(0))->getSExtValue();
// Match: // Match:
// Index = step(const) << shift(const) // Index = step(const) << shift(const)
else if (Index.getOpcode() == ISD::SHL && else if (Index.getOpcode() == ISD::SHL &&
Index.getOperand(0).getOpcode() == ISD::STEP_VECTOR) { Index.getOperand(0).getOpcode() == ISD::STEP_VECTOR) {
▲ Show 20 Lines • Show All 4,236 Lines • Show Last 20 Lines