anoopkg6 wrote: [like] Anoop Kumar reacted to your message: ________________________________ From: Ulrich Weigand ***@***.***> Sent: Friday, April 25, 2025 2:30:52 PM To: llvm/llvm-project ***@***.***> Cc: Anoop Kumar ***@***.***>; Author ***@***.***> Subject: [EXTERNAL] Re: [llvm/llvm-project] Add support for flag output operand ***@***.***" for SystemZ. (PR #125970)
@ uweigand commented on this pull request. Not a full review, just some initial comments on combineCCMask. I think it would be good to have more comments explaining the specific transformations you're attempting to implement, with an argument @uweigand commented on this pull request. Not a full review, just some initial comments on combineCCMask. I think it would be good to have more comments explaining the specific transformations you're attempting to implement, with an argument why they are correct for all inputs. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060312364 >: > return false; - // Optimize the case where CompareLHS is a SELECT_CCMASK. - if (CompareLHS->getOpcode() == SystemZISD::SELECT_CCMASK) { - // Verify that we have an appropriate mask for a EQ or NE comparison. + // Optimize (TM (IPM (CC))) Adding a case to optimize (TM (IPM)) in addition to (ICMP (IPM)) does make sense in general. However, you need to take care that the optimization is correct for all possible inputs to TM, not just the ones the come up in the "good case" you're looking at. That doesn't appear to be the case here. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060314372 >: > bool Invert = false; - if (CCMask == SystemZ::CCMASK_CMP_NE) + if (CCMask == SystemZ::CCMASK_TM_SOME_1) Invert = !Invert; There's four possible CCMask values for TM. It doesn't look all four are handled correctly. (You can of course bail out if there's some mask value you don't support, but you shouldn't make any silent assumptions.) ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060316349 >: > Invert = !Invert; - else if (CCMask != SystemZ::CCMASK_CMP_EQ) + auto *N = CCNode->getOperand(0).getNode(); + auto Shift = dyn_cast<ConstantSDNode>(CCNode->getOperand(1)); Calling the operand of TM "Shift" is confusing. There's no shift happening here; TM basically performs an "and" operation. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060318970 >: > return false; - - // Verify that the ICMP compares against one of select values. - auto *TrueVal = dyn_cast<ConstantSDNode>(CompareLHS->getOperand(0)); - if (!TrueVal) + if (N->getOpcode() == SystemZISD::IPM) { + auto ShiftVal = Shift->getZExtValue(); + if (ShiftVal == (1 << SystemZ::IPM_CC)) + CCMask = SystemZ::CCMASK_CMP_GE; Well, what if the second TM operand is anything else? You'll still do the optimization here, which is likely to be incorrect. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060320864 >: > + CCMask = SystemZ::CCMASK_CMP_GE; + if (Invert) + CCMask ^= CCValid; + // Return the updated CCReg link. + CCReg = N->getOperand(0); + return true; + } else if (N->getOpcode() == ISD::XOR) { + // Optimize (TM (XOR (OP1 OP2))). + auto *XOROp1 = N->getOperand(0).getNode(); + auto *XOROp2 = N->getOperand(1).getNode(); + if (!XOROp1 || !XOROp2) + return false; + // OP1. (SELECT_CCMASK (ICMP (SRL (IPM (CC))))). + // OP2. (SRL (IPM (CC))). + if (XOROp1->getOpcode() == SystemZISD::SELECT_CCMASK /*&& + isSRL_IPM_CCSequence(XOROp2)*/) { I don't even fully understand what optimization you're attempting here - but this code completely ignores Op2, which is obviously incorrect. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060325910 >: > + int CCValidVal = CCValid1->getZExtValue(); + int CCMaskVal = CCMask1->getZExtValue(); + if (combineCCMask(XORReg, CCValidVal, CCMaskVal)) { + // CC == 0 || CC == 2 for bit 28 Test Under Mask. + CCMask = SystemZ::CCMASK_CMP_GE; + CCMask ^= CCMaskVal; + if (Invert) + CCMask ^= CCValid; + CCReg = XORReg; + return true; + } + } + } + } + // Optimize (AND (SRL (IPM (CC)))). + if (CCNode->getOpcode() == ISD::AND) { Here is starts to look very confusing. The operand of the combineCCMask routine has to be some Z instruction that sets the condition code - the whole routine is about analysing condition codes set by prior instructions! A plain ISD::AND does not set any Z condition code at all, it simply has a regular (integer) output value - how would it ever be a possible input to combineCCMask? What does this even mean? ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060327291 >: > return false; - if (CompareRHS->getAPIntValue() == FalseVal->getAPIntValue()) - Invert = !Invert; - else if (CompareRHS->getAPIntValue() != TrueVal->getAPIntValue()) + // Bit 28 false (CC == 0) || (CC == 2). + // Caller can invert it depending on CCmask there. + if (ANDConst->getZExtValue() == 1) { + CCMask = SystemZ::CCMASK_0 | SystemZ::CCMASK_2; + CCValid = SystemZ::CCMASK_ANY; + return true; + } + return false; + } + // (SELECT_CCMASK (ICMP (SRL (IPM (CC))))) + if (CCNode->getOpcode() == SystemZISD::SELECT_CCMASK) { The same comment as above - SELECT_CCMASK (while at least a Z specific opcode) does not itself set the condition code (it uses it, of course), and so it cannot be an input to combineCCMask either. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060327821 >: > - auto *NewCCMask = dyn_cast<ConstantSDNode>(CompareLHS->getOperand(3)); - if (!NewCCValid || !NewCCMask) + int CCValidVal = CCValidNode->getZExtValue(); + int CCMaskVal = CCMaskNode->getZExtValue(); + SDValue CCRegOp = CCNode->getOperand(4); + if (combineCCMask(CCRegOp, CCValidVal, CCMaskVal)) { + CCMask = CCMaskVal; + CCValid = SystemZ::CCMASK_ANY; + CCReg = CCRegOp; + return true; + } + return false; + } + + // Both oerands of XOR are (SELECT_CCMASK (ICMP (SRL (IPM (CC))))). + if (CCNode->getOpcode() == ISD::XOR) { And once again an ISD::XOR cannot be an input to combineCCMask. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060344904 >: > + if (!RHS) { + SDValue CmpOp1 = CCNode->getOperand(0); + SDValue CmpOp2 = CCNode->getOperand(1); + auto *CmpNode1 = CmpOp1.getNode(), *CmpNode2 = CmpOp2.getNode(); + if (!CmpNode1 || !CmpNode2) + return false; + if (CmpNode1->getOpcode() == SystemZISD::SELECT_CCMASK || + CmpNode2->getOpcode() == SystemZISD::SELECT_CCMASK) { + SDValue CmpOp = + CmpNode1->getOpcode() == SystemZISD::SELECT_CCMASK ? CmpOp2 : CmpOp1; + SDNode *SelectCC = CmpNode1->getOpcode() == SystemZISD::SELECT_CCMASK + ? CmpNode1 + : CmpNode2; + int CmpCCValid = CCValid, SelectCCValid = CCValid; + int CmpCCMask = CCMask, SelectCCMask = CCMask; + bool IsOp1 = combineCCMask(CmpOp, CmpCCValid, CmpCCMask); This calls combineCCMask on some random operation that does not set a condition code - is this why you end up in some of those cases above? That doesn't make sense. What is the actual optimization this code path is supposed to achieve? ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060358072 >: > } + int CmpVal = RHS->getZExtValue(); + // (BR_CC (ICMP (SELECT_CCMASK (CC)))) + if (LHS->getOpcode() == SystemZISD::SELECT_CCMASK) { + int CCVal = RHS->getZExtValue(); + int Mask = CCMaskForICmpEQCCVal(CCVal); + bool Invert = false; + if (CCMask == SystemZ::CCMASK_CMP_NE) + Invert = !Invert; + SDValue NewCCReg = CCNode->getOperand(0); + if (combineCCMask(NewCCReg, CCValid, CCMask)) { Again a recursive call with an opcode that does not set CC. ________________________________ In llvm/lib/Target/SystemZ/SystemZISelLowering.cpp<https://github.com/llvm/llvm-project/pull/125970#discussion_r2060358436 >: > + if (LHS->getOpcode() == SystemZISD::SELECT_CCMASK) { + int CCVal = RHS->getZExtValue(); + int Mask = CCMaskForICmpEQCCVal(CCVal); + bool Invert = false; + if (CCMask == SystemZ::CCMASK_CMP_NE) + Invert = !Invert; + SDValue NewCCReg = CCNode->getOperand(0); + if (combineCCMask(NewCCReg, CCValid, CCMask)) { + CCMask |= Mask; + if (Invert) + CCMask ^= SystemZ::CCMASK_ANY; + CCReg = NewCCReg; + CCValid = SystemZ::CCMASK_ANY; + return true; + } else if (CCMask == SystemZ::CCMASK_CMP_NE || + CCMask != SystemZ::CCMASK_CMP_EQ) { This condition looks incorrect. — Reply to this email directly, view it on GitHub<https://github.com/llvm/llvm-project/pull/125970#pullrequestreview-2794248049 >, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BM5K4GTSFUQ33NTN6K4DE4D23JBJZAVCNFSM6AAAAABWSJVEQKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOJUGI2DQMBUHE >. You are receiving this because you authored the thread.Message ID: ***@***.***> https://github.com/llvm/llvm-project/pull/125970 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
