Changeset View
Standalone View
lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
| Show First 20 Lines • Show All 2,758 Lines • ▼ Show 20 Lines | SDValue Extract = CurDAG->getTargetExtractSubreg(SubReg, SDLoc(Node), VT, | ||||
| Node->getOperand(0)); | Node->getOperand(0)); | ||||
| DEBUG(dbgs() << "ISEL: Custom selection!\n=> "); | DEBUG(dbgs() << "ISEL: Custom selection!\n=> "); | ||||
| DEBUG(Extract->dumpr(CurDAG)); | DEBUG(Extract->dumpr(CurDAG)); | ||||
| DEBUG(dbgs() << "\n"); | DEBUG(dbgs() << "\n"); | ||||
| ReplaceNode(Node, Extract.getNode()); | ReplaceNode(Node, Extract.getNode()); | ||||
| return; | return; | ||||
| } | } | ||||
| case ISD::Constant: { | case ISD::Constant: { | ||||
| // Materialize zero constants as copies from WZR/XZR. This allows | // If all uses of zero constants are copies to virutal regs, replace the | ||||
gberry: Would it not make sense to replace any use of constant 0 with wzr/xzr when it is legal? | |||||
haichengAuthorUnsubmitted Not Done ReplyInline ActionsI agree with you. I started with replacing any constant 0 with wzr/xzr, but triggered a lot of assertions. For example, wzr/xzr is not expected to appear as the condition of a conditional branch. Then, I narrowed down to CopyToReg only, but still triggered some assertions when copying wzr/xzr to another physical register. Now, I narrow down to the most common situation, copying constant zero to a virtual reg, no assertion is trigged and performance looks good. haicheng: I agree with you. I started with replacing any constant 0 with wzr/xzr, but triggered a lot of… | |||||
gberryUnsubmitted Not Done ReplyInline ActionsYeah, that's why I said "when it is legal" :) gberry: Yeah, that's why I said "when it is legal" :)
It made be too much of a pain to determine… | |||||
haichengAuthorUnsubmitted Not Done ReplyInline ActionsAnd I check if all the uses are copies to virtual regs and call RAUW to replace because SDUse::set() is a private method. Do you think it is worthwhile to change the API to be public? It does not make big difference to the performance since most of the time constant 0 has only one use. haicheng: And I check if all the uses are copies to virtual regs and call RAUW to replace because SDUse… | |||||
gberryUnsubmitted Not Done ReplyInline ActionsNo, I'm not suggesting you change SDUse (that is private for a reason). If you want to do this experiment you would need to replace the users themselves with new nodes that read XZR/WZR directly. I think either approach is okay here, but you should probably wait for someone else to approve it. gberry: No, I'm not suggesting you change SDUse (that is private for a reason). If you want to do this… | |||||
haichengAuthorUnsubmitted Not Done ReplyInline ActionsI tried and it seems I cannot replace the users themselves with new nodes that read XZR/WZR directly. The new node I can create here is CopyFromReg and its type is MVT::i32/i64. The user is CopyToReg and its type is MVT::Other. The types do not match and I cannot do the replacement. Another potential issues is that the users can be used in the other MBB because it is lowered from a PHINode, but use_iterator seems not include this usage. I think I would stick to the current approach which just replace #0 with XZR/WZR because it is simple and conservative but covers the most common situations. If I miss any coalescing opportunities, they would be coalesced anyway in the later pass. haicheng: I tried and it seems I cannot replace the users themselves with new nodes that read XZR/WZR… | |||||
gberryUnsubmitted Not Done ReplyInline ActionsI'm not sure I fully understand the problem you're running into trying to do this forward substitution on only some uses. If you wanted to try that you would leave this node alone and instead replace the use nodes that are vreg COPYs that are reading the COPY of XZR/WZR. gberry: I'm not sure I fully understand the problem you're running into trying to do this forward… | |||||
| // the coalescer to propagate these into other instructions. | // conatants with WZR/XZR. Otherwise, materialize zero constants as copies | ||||
gberryUnsubmitted Not Done ReplyInline ActionsTypo: 'conatants' -> 'constants' gberry: Typo: 'conatants' -> 'constants' | |||||
| // from WZR/XZR and allow the coalescer to propagate these into other | |||||
| // instructions. | |||||
| ConstantSDNode *ConstNode = cast<ConstantSDNode>(Node); | ConstantSDNode *ConstNode = cast<ConstantSDNode>(Node); | ||||
| if (ConstNode->isNullValue()) { | if (ConstNode->isNullValue()) { | ||||
| if (VT == MVT::i32) { | if (VT == MVT::i32 || VT == MVT::i64) { | ||||
| SDValue New = CurDAG->getCopyFromReg( | unsigned ZReg = (VT == MVT::i32) ? AArch64::WZR : AArch64::XZR; | ||||
| CurDAG->getEntryNode(), SDLoc(Node), AArch64::WZR, MVT::i32); | SDValue New; | ||||
| ReplaceNode(Node, New.getNode()); | if (llvm::all_of(Node->uses(), [](SDNode *User) { | ||||
| return; | return (User->getOpcode() == ISD::CopyToReg && | ||||
| } else if (VT == MVT::i64) { | TargetRegisterInfo::isVirtualRegister( | ||||
| SDValue New = CurDAG->getCopyFromReg( | cast<RegisterSDNode>(User->getOperand(1))->getReg())); | ||||
| CurDAG->getEntryNode(), SDLoc(Node), AArch64::XZR, MVT::i64); | })) | ||||
| New = CurDAG->getRegister(ZReg, VT); | |||||
| else | |||||
| New = CurDAG->getCopyFromReg(CurDAG->getEntryNode(), SDLoc(Node), | |||||
| ZReg, VT); | |||||
| ReplaceNode(Node, New.getNode()); | ReplaceNode(Node, New.getNode()); | ||||
| return; | return; | ||||
| } | } | ||||
| } | } | ||||
| break; | break; | ||||
| } | } | ||||
| case ISD::FrameIndex: { | case ISD::FrameIndex: { | ||||
| // Selects to ADDXri FI, 0 which in turn will become ADDXri SP, imm. | // Selects to ADDXri FI, 0 which in turn will become ADDXri SP, imm. | ||||
| int FI = cast<FrameIndexSDNode>(Node)->getIndex(); | int FI = cast<FrameIndexSDNode>(Node)->getIndex(); | ||||
Style nit: you can reduce the level of indentation below by inverting these if tests and doing an early break after each one. gberry: Style nit: you can reduce the level of indentation below by inverting these if tests and doing… | |||||
| unsigned Shifter = AArch64_AM::getShifterImm(AArch64_AM::LSL, 0); | unsigned Shifter = AArch64_AM::getShifterImm(AArch64_AM::LSL, 0); | ||||
| const TargetLowering *TLI = getTargetLowering(); | const TargetLowering *TLI = getTargetLowering(); | ||||
| SDValue TFI = CurDAG->getTargetFrameIndex( | SDValue TFI = CurDAG->getTargetFrameIndex( | ||||
| FI, TLI->getPointerTy(CurDAG->getDataLayout())); | FI, TLI->getPointerTy(CurDAG->getDataLayout())); | ||||
| SDLoc DL(Node); | SDLoc DL(Node); | ||||
| SDValue Ops[] = { TFI, CurDAG->getTargetConstant(0, DL, MVT::i32), | SDValue Ops[] = { TFI, CurDAG->getTargetConstant(0, DL, MVT::i32), | ||||
Not Done ReplyInline ActionsThis could use a comment explaining why it is being rejected. gberry: This could use a comment explaining why it is being rejected. | |||||
| CurDAG->getTargetConstant(Shifter, DL, MVT::i32) }; | CurDAG->getTargetConstant(Shifter, DL, MVT::i32) }; | ||||
| CurDAG->SelectNodeTo(Node, AArch64::ADDXri, MVT::i64, Ops); | CurDAG->SelectNodeTo(Node, AArch64::ADDXri, MVT::i64, Ops); | ||||
| return; | return; | ||||
| } | } | ||||
| case ISD::INTRINSIC_W_CHAIN: { | case ISD::INTRINSIC_W_CHAIN: { | ||||
| unsigned IntNo = cast<ConstantSDNode>(Node->getOperand(1))->getZExtValue(); | unsigned IntNo = cast<ConstantSDNode>(Node->getOperand(1))->getZExtValue(); | ||||
| switch (IntNo) { | switch (IntNo) { | ||||
| default: | default: | ||||
| break; | break; | ||||
| case Intrinsic::aarch64_ldaxp: | case Intrinsic::aarch64_ldaxp: | ||||
| case Intrinsic::aarch64_ldxp: { | case Intrinsic::aarch64_ldxp: { | ||||
| unsigned Op = | unsigned Op = | ||||
| IntNo == Intrinsic::aarch64_ldaxp ? AArch64::LDAXPX : AArch64::LDXPX; | IntNo == Intrinsic::aarch64_ldaxp ? AArch64::LDAXPX : AArch64::LDXPX; | ||||
| SDValue MemAddr = Node->getOperand(2); | SDValue MemAddr = Node->getOperand(2); | ||||
| SDLoc DL(Node); | SDLoc DL(Node); | ||||
Not Done ReplyInline ActionsI think you can get rid of this if/else by re-writing as: SDValue New = CurDAG->getNode(ISD::CopyToReg, SDLoc(Node), Node->getVTList(), makeArrayRef(Ops, NumOperands)); gberry: I think you can get rid of this if/else by re-writing as:
```
SDValue New = CurDAG->getNode… | |||||
| SDValue Chain = Node->getOperand(0); | SDValue Chain = Node->getOperand(0); | ||||
| SDNode *Ld = CurDAG->getMachineNode(Op, DL, MVT::i64, MVT::i64, | SDNode *Ld = CurDAG->getMachineNode(Op, DL, MVT::i64, MVT::i64, | ||||
| MVT::Other, MemAddr, Chain); | MVT::Other, MemAddr, Chain); | ||||
| // Transfer memoperands. | // Transfer memoperands. | ||||
| MachineSDNode::mmo_iterator MemOp = MF->allocateMemRefsArray(1); | MachineSDNode::mmo_iterator MemOp = MF->allocateMemRefsArray(1); | ||||
| MemOp[0] = cast<MemIntrinsicSDNode>(Node)->getMemOperand(); | MemOp[0] = cast<MemIntrinsicSDNode>(Node)->getMemOperand(); | ||||
| ▲ Show 20 Lines • Show All 1,205 Lines • Show Last 20 Lines | |||||
Would it not make sense to replace any use of constant 0 with wzr/xzr when it is legal?