[llvm][RISCV] Implement Zilsd load/store pair optimization #158640
Conversation
This commit implements a complete load/store optimization pass for the RISC-V Zilsd extension, which combines pairs of 32-bit load/store instructions into single 64-bit LD/SD instructions when possible. Default alignment is 8, it also provide zilsd-4byte-align feature for looser condition.
| @llvm/pr-subscribers-backend-risc-v Author: Brandon Wu (4vtomat) ChangesThis commit implements a complete load/store optimization pass for the Patch is 53.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/158640.diff 12 Files Affected:
<!DOCTYPE html> <!-- Hello future GitHubber! I bet you're here to remove those nasty inline styles, DRY up these templates and make 'em nice and re-usable, right? Please, don't. https://github.com/styleguide/templates/2.0 --> <html> <head> <title>Unicorn! · GitHub</title> <style type="text/css" media="screen"> body { background-color: #f1f1f1; margin: 0; font-family: "Helvetica Neue", Helvetica, Arial, sans-serif; } .container { margin: 50px auto 40px auto; width: 600px; text-align: center; } a { color: #4183c4; text-decoration: none; } a:hover { text-decoration: underline; } h1 { letter-spacing: -1px; line-height: 60px; font-size: 60px; font-weight: 100; margin: 0px; text-shadow: 0 1px 0 #fff; } p { color: rgba(0, 0, 0, 0.5); margin: 10px 0 10px; font-size: 18px; font-weight: 200; line-height: 1.6em;} ul { list-style: none; margin: 25px 0; padding: 0; } li { display: table-cell; font-weight: bold; width: 1%; } .logo { display: inline-block; margin-top: 35px; } .logo-img-2x { display: none; } @media only screen and (-webkit-min-device-pixel-ratio: 2), only screen and ( min--moz-device-pixel-ratio: 2), only screen and ( -o-min-device-pixel-ratio: 2/1), only screen and ( min-device-pixel-ratio: 2), only screen and ( min-resolution: 192dpi), only screen and ( min-resolution: 2dppx) { .logo-img-1x { display: none; } .logo-img-2x { display: inline-block; } } #suggestions { margin-top: 35px; color: #ccc; } #suggestions a { color: #666666; font-weight: 200; font-size: 14px; margin: 0 10px; } </style> </head> <body> <div class="container"> <p> <img width="200" src="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAZAAAAGZCAMAAACQbpc2AAADAFBMVEWEBz6FAD6FAD6GAD+MAEGOAEOKAEGOAEOGAD+IAECOAEOOAUOOAEOOAEOOAEOOAEOOAEOOAEOCACyOAEQAAACKAEJpoJ2KADqu0eSKAD2BAD+KAD6AxCNqwoX1Ziawo9LYnGuwhL7aiwaIyYudst6DlrnhcWvCcVvlbE9PvsL2u3uYmdCZum9Rns7clUYsreOYp2nuj37kiZLUfqhvxafqfmrNWEjA22uz1Vu6lsiRAArWhLfgdoONnF36rWrzpz/ajy/LbqR8pdWDVXx8faN6m0+6hE3+1pmHP2mTy1b4k1p/ueFlj7vMfzyBs1v/2GfLpFL5rQD+7teQABj4n0/rpCz6r1Z8apP+4rrfw1iV0JyLUkqQACTDnah5AACtaEt9zcGNhleJbU5lsOFRvE5FuOPS4WLSt7+8ajvXcJpcyNvhztSwUkTGgZz9uDSxRnPZwsnTkqy7jpsPt/AxyvnAeJMAxPnMiqPJYIyzf4/KqrPedaK5bIsQwfTo2t72hUepPWvn6mO8VoXrgbDRaJXle6r75FzZm7S2TnyTNUXv5eiDAA6j0WmKyWv51liRAACjP0OZNVuwa4L+uQAcw/SiNGH57mIxxPKmU3DgpL2sX3rutc3/vh9gu1D3tNEAtvOSKVAmxfagRmbnrcWZJVaTJET2qsv84eyk04PzcEb71OX0u9OUzHD1nsR+1PhtwFUBvfQ5x/RizvdQy/VZzPWaz3b8uST+wFWDABqKAC6m03uGx2X+xGKBxmL2iWDQjb777/Nt0Pf+xmlCyPT+v032hVv+yXD3lGx2wlr99/hJyvX1gFV20vf1e1DCaqr6xdz+wlzJe7T2jmWFACT3i7v+0IP9u0R8xF7Oh7r+zHn/+WLHc7CQGUP3vtePEUT0i7qHADORIUyEAC2BAACf0Hr3j73zhLX7x96RGkrMgbePFEiRDUeOA0P4wNiVAEaPBkTzh7f9yeCPCkSLADmRAESYAEf+/f6CAET4xdz4wtnzibiKAEH/9GH///+OAEP4w9pHeYEoAAAAFXRSTlP9+PLorFm9i97MnGlJOnoNKxwBAwB644ahAACClklEQVR42uydf0xUZ77/rQLyS4TBk4Yh2XTdxLrurr3qVcyuRdzvt7CSRYSrLMS7S/R6WZqy9Mem2dt+myLG2E4VOnXipLWYGnXStTXx/lOUq/QSgwgxWpEgFccTkE0cYZyJZ2YcyeDj9/15nvODmQG1OpR1t+8znBlmEPW85vPz+ZwzM+b9A8o0b85c7J5K/SMCMc2bG/cDkL8jZc5LmTnvByB/N8qcl/lM4rynVf+QQGYHEp5WA/nHA2LKnBfv9yQDy9OpfzQg4JHKmCftByB/H8o0gYc/5In/AchjKTMz1uYxL83vlxhL/wHIY/HALbY4MhICjDH/M09tGQIg04gjIw0HzhQ7HJlpIY+kuCVPwlNrIAAynUQSslNxFwvXB6xzU2YF/JJTYu7A0xvTpxnInIAnLoMi8RPIZOKRKD1tJnDITrPVEfLP+QHI4/WcMmf5A+ZUHNMngQFlpCRIwOF2SOb1FZJ/5twfgDx2TR3ye2bj0eOwEDAy01MSQoEAAw7ZbKnoX292xD29PKYTCL27Uz1MYoE42Mp3+4PEQsBInsUCMA5Jdihmtn5n/1hl0PX0ViEAMr0571yzn7FQIAk+5pFRmMTDjNS0OA6DSYrT4ZTMLL+xf2xsp9s/4yn2WNNpIXMyKM/ySCDiSXz4MRQuijQ3PT4tLjHkGRkZdkIOJwuZg9b1jf3gQR7r6W1kTScQpFjPxKOwDgAIUzyzQOQRYMydkzI7YZbidDkcTkUKQWazOeitXF9hBw5SZciZOi9T6GnkMp0uKymQkDHHAxzMLwcSM+eZJqWhskhOMivDDscIC5ohyW+x1lTm56+v2DkGGlz9FSEnynRdgPKUlezTCSTe4zGnZfsVtmWT34kljAmJgAbBSEuYyQIjLngnkLBUgkJFY6PdbgcKaExTfz48Fnxaanx8fGp6hlY1fvcU7p8SSEY288NAnP6XtlpgI7MncFq8Ao+Pm+kPBEacTsBglfkVjWQRYSB0Ho2S7ElOfoZ5SMycmJySnvndmWQCyT8fECghEGKMyf4tW/fLTKGeeXR/ak7yMx5kUk7UfCFrPrwTBzGZYCDMjx/nAm7UJ7OSU+cK5/XoPOaiWP3nA5IpIjqAbNq69SXcmTPCDwPhSPBTBU7GYcmvsE/KQosgQSdjjKcJsux0yrLiBh/PTJ2J6dHsIwHW+s8IZI6faUC2boHTQoUd5q0y4uDSQgwVeKiyYueDYWgpFv1CpGBMQg4mSRJz4puRgH9mcnyGFuVND6lWM+MCqdMHxDRtiQjGp2YSEYVZtkIWOC3jONCDlFCA4Yg6zVJ+xdhDcYgahCmoEXkGVlNZWWN1MkqLJRlQPNkJaZif06mYJsvn5swKZGdMs8uanozdhCAiERDHfgB5yS97jNyX3qcev4TjGwrmNz6chojoXtnBzKGa/IqKnTvtY2P2nTsbKyrWV1qAKCQ7XDAUQMkwahsio24kXq7G+f2epOkM6nNS00X8nJbeIoB4CQi0CU4rBU+qKVgivYZ3eGUFcDwKD7tVAg6ZQg2JPyce7axYn29FNe/m3suclJwyJwO2EiVk14nkJOmfMW1AZgf8ocS0dEIyHZUIB8L2b91PcV2hxrmJv5L+jAcZmCPE1o89Eg6o0mzBj0eHGsEFUCoZvJfT6UXu5Q/NSkpOS0mdk56eQUqfk4oWALJranb6zXOnEUjIT9mhFJf6/TsuHHYGuQkIEdnEhImQfZjBQ3GaKxsfFQdKQgt+fFJrIktpXF/p50wU/J+BJeDxM958CYVwDHifUmJM4hXRtAHhh8QpBzxJQPK9hnfR7NUsZP9+nvrOQhRBtJ8FHrJsflTzgNaDx8N+nDPJl4kJoLhDEtUqQgwsFIDiSp9OINl+5pWC+CcGPAlzvm8kpkSPZADZryZaJl4xInOteGTzGMsnHv0TEIgQjyj5Fkq9FIfDAS4K/n63V0Z6zJCOMTatHgtAkjyKZC2qZsGQ0+OPywCS7zvNEkBwQy3i5LVICp51St+Bh70SPPLDn7QXFzeiyRItNaBYmRkiK5GBQUK/0lJdJDPMrCRN5+j8jLQAgDQ1lVR7g4ocCM2e+71Fd2KfHJB42svFwzpKgAzENVnYR7/dbhzIyfNdazCKB0Rp785JA4q9sWL9+spKi5ehfvRb0cFvrK0J4u+d3pmVGRlw1sG8pt7akupg0KkEZsZ/f9Edea8BhCQy37iApCiCR/3KMV1IZyd2V+slyYn4Ea0HV5MiNwazRlgSUmXBAy4rfVqB4L+vSI6S8q4C2/wahBK/JyH9+/JbwjmhUhc4hM9KTkeEdZo5j6pj4wwk34qKJBpHRaUZgz/48cdTv6qdl2urqRM2zR6LCkMGE6mxld4r7bXNPxCUFQ+D38o0fT+FCIDIbJNmITzPSvJIDgRo4rG1qn9c59AczN/ZH+mt8s0wD3r+iQQcnAcUmN4RiRncjzuD1SByr7QDfktyugOzUr8PIzFpQLbs1+RAiEevPVQ5BtVvXRt27GE3DlhCmLdiqAa9FXj0BLI3AofKQ/IkmmAg0wqEYqgTYQRE7pRfnm9FcPfQQOHUB3eMAfEFKhXIMQQR4IEPk/k7fu3++sjWiES1hoGo0ux0mCvxw/A8IohDD44d/WHwKIi0A8fl2hrw0AxE6LGWDU1PDEQsS3iV4Hxb+b1797oKSuZLZCShFCpKvhcg7CURQVaupSDCmEMNIFtXRjbXZVkcf+H8K1jQInorKC4acVw1XR6X79p7oStXOjpOFyMPJrW341UVRiOncfnyaqvgIXmSI0aGv5ue3Lpm0G9AeaZIbKOtoAtISq/ASJjbH0iKSXA3PRSIIoAc21q1EomvU3NY9tZj9ZHNEU7L0tjPDQbfar0VHFeoVkfSXFvbvnNcRcKhgMrp06cuXqQVYA6jkf4I12IMaTMuf3JaMjQ7LX5Ohuk7M8ESzpOupMzQFopkybnRdrm87c69e+XNRU4qkGJjJCbTg4EozLlfBVJFlYhby7D27x6LBgIiEsPrqD3MliCZB+GALpPw5odxUOs9SnZV3LM1EkFDRUF4SlWegJAHPVexqGV6ZCgmeJuUJwfCnVYIRCy1NlvTabyXmppqeAcuBkaSiXaM6YFZFpIsDUj9VkR1p1ri7T5GKVa9PRIIbCi0Hu5KcqjmcfE4VyNh6NfE/Zi4x00vE8EL7MIFdyUrDHK7FS6v4nZLErHxhBJStIXGR5vb8MfAQuiAJVHzKGjdWNtks9lK8qySU0Jjx+2R0shIniRKxKHufWAd4kSSdQwbgIwdQ1TnEQQkWhtoiWlllIUoblTTlRILSdw87HBCqheiw32xcUIdP86tSLMjfOlWVY2Q6aUleKIiCTE8gRV5rxtN4FBSSvojjXjBPuYmBubEAAhvd/sl2Ii7prq6usYVlKSgdXFNUIGRJFLH8UkGGSbP60U+ISOEgAeA1I8hqushvTOHEi1YSTgQ77PZI4qMtXO1M3/61KliO0X1i7CUSLVjK2sva48QojrEjWWxIyg7QTgISQztRovV4nCIJyTJjTEJLJ9IibNT52qrjJPi4ItqofQnDOszxs8byCwIcRx5JbaSIgmG4mFPYiS0BkVWPHnrBOuFZB6EpL7fADKWcxIhvaovIoaAx8HDA8MIJBaRbBWfLraTnTRe1HScfxlQeFDhAI7rQI5z8wAOpC9OHHtmrSkqWrx49Wrhw6DFi4uqayyMDgagYBDpmYQUbUU+OiUWnOLNWG3MiIGF6E0M5oXlopNRndfc3lt+xbYRySAZyeNHEoRtP/ULMydzaAGUhceOcR7H6vtXbt3MHKKN/hWF9FYYSDiQ4RNvHjo84II153MQHcU8PFzUJXCoTODL9LiCn+ZMDAGHhKZyTdFigAChKIFLdY0TUGAp3gBfkU/N0BkY4t+nJiEP4EueMQAiGq8hxtD53bixuQllO1WJNl64hx7fSExkeqEHdIcSPahCAISYdNoB5CUViP1oFQzkZNSQz3DPO7cPfdTiYsh4QaT4Cn8FceQUtjABB5+qE1lVI32PO/gwAWV1kQU+KsgqF7dr4YXvoqksLrLCUshQ/AGE+cS4NAypAoKhzIzU2bM8yOAZH4+IERAK7JT9Vjc1NbV3tN1D/tvWa8s7EHR6Kd16nMLdJHxhaJKGNjUJ0OolGPTVNx5I/Vd2JFo5/eGVukxA7t749KPrLuYgIlfs3G+dIgHJzmIDSCO9BEcFFFEqW1zpJd9sLVrdLnjwndgmglLEDQXug09D0pBqUtzstLSUlJS0tOQEnBpB43xUV2I5JxZAoEyxbopGY3Wz7VT5ZZTtVJJQV57JIU/osYxkrhqcJHS0Jw4hTngsof1rEcl1IFU5gHLUHtY6sQggN27d/hRxhBPppRd2nlI1tvOUbinFO4vVuKI7MIOGg4bnrdWLEUzCdVm9TWQo1RYwUZCMSRKw8DV5be9H/Hcib8ZaSsyAiMkCxBE0GmttBaUdvV2EpA1deSWoSI9Tk2CIIY4DmfjEcbzMy3QVCBol4yykqp5DCW8uMoUDuX/r9qHhZwWRfsNAisGDK8p7CSoQYBTVSHx+vmhDmZqGRSOZDMriaqukQqFCxdiwAsyCCpB44mMHhA5QyK8TuVN+uhxI0Eo5XlIjKeyxCneTOS6VWwgtlUfxyEz0UEg3gIwZQKggzFENxDj1ww0ge765DyKfZJG34zWk/TQ6Iqe5fYxXZFQBjOexbCs5avIXA4ahaCrERduinZeDoDCFZoe5cIcYw6qLPJI7lBGjGKIlv0wncqqtq7y3vPQO7zf6mZtJ/u8aSUwmxKVZjORJnHj9FkWIDqRqPBBiEdHKqqAyZPj6PgABkfdaRtycSH8xgACJfYwwRDDRoZQtzn/++fzFi8vKhKlMJI1LNJzIKF9tlUEFUvfOmqLVq1cEFf7fjB0QE09SmUqkpABICnoLCgpO5QUVJMSK9F1bwCaktX5GksiWo6t03UAgWoxaqwKZdHKXgNwnjd7+S88wIyIqkGJxX1xs8CiGxnswPBzvvjQqZWUbNmyo49qwQWeiP7sB1QnHEkEFWKproOoinjavPjIisRhMdM2IrOP8TCXSbLN1lKMYsZXMd0hOairI0ndpARu5tNsbHUWIPV6AgWhCXYgVEAJSMTGQfGEhH98XuvFOj4sRPgFkbCfngb1wYMU7kWMRj0njynHuxzbUHSH96Ec/OoKDX0ZmAhhnL3195sy58+ePcOEVqlSwTSLgOFITxNranBgDMXEi3GvV5K2upb4WnGYQ37L5eY6g020U7o8+VeLms3AM7i4iXjEjxYJQF9o7j00GBKqUCEiLCmT0m2sHkWqhN/xcB5EYO63viYi9385pPDCslAGGKmKh62KZrg1cZCiiBUZbNA78nhWSmztmU0yBCBuhOCJL1E/AQD8LSohi1SUdtpLKoFeW1Jrk0adK4OktzGvUIkbzTGaWcTxQF9bvBxCnFD0+ypNbi6S42cjAwVt3CQilWlkgjb4WiMBVGW6LwNiLBZgHhJUN9P4/r9GguBId6R8eUlYTDeLB3CLHijEQk07EyXg2IaOCss7vxQzERRs1t1jokdMt4bLQXsfsghjmMHjMRIbNI3onNsLByxAAwbM7JwTSKCler3vEt/fLu6Mqkdd3ueitM5Lb0VHcAdnHNB4CDIhMRIWCCXBwHTlft+G48GBim0hEQTWWOtKRuvP4Eg+FaiS3m0YeYw5EeHdGRJhbcRISR/X806X3IBoUoqUDGEnCI6y4G4nUlk5aKjf6oOCBGlQ4LEGDA0GSBSB+yaplu1FJljubjTx7+MtvAITrm30URpyhGg4EBkJ7bAKHweRUFJWyc+fPnQOPcxvKLhqKhgKnRRAA7kFa4Qh63XyACIo5EOo/wQoYNbaq8/I2NjfbLtP8AxLg0nbe3HrUmkSMijrZ5s7NcESiDW/wIIfVaWz7V9JUw7HNfnPlpKdHtbx10BdyfaoBGb3xqStLARHz8x3CQPgdoNAWrjAqFwvPQefPFZZxY4kWmABEoQ6i7gE0FgIH6rRYTc3PmLjCNhMRRappbmpqtp0uKCmnZiM3Eortj1iTiNV6OKZOOCLxzzV4KIyt6+zUeBCQ/noYyxZ/MH+SJCvoOPHKWyeG/Z/eABA19339hIsRkeeugAN6v7pO020iJNjlnoHOnQENUllkqIdRFJ7TTOJ8YVlZO+/IA4p4EnvDNlZY/UEJ5kHypE4REJN6zJiMNcQmWEhpqY17LTKS2hJkXW6m1iQPP4kQYXdd5zoMMdPFjU06Dzc4GTzwCECq9nce2+QPvUhAqqLWxGukbG/bG0MuZgBBqrWHqhE5ZO0AkTEeSMKgRFMhHl9/faYwV/VdG8rGZ2Aai/OqCnNz8RR8FsSfFUYBrVhRY3UoEtXsXsbFW++xBmJ0GpMCPPu1gIjNVtrVa0NcJ+FREXPzwj2BLgb3QCDpfC4Rh3sTUzx06oeI52BNjox0TLvtr+pfCyAWv7SOL91GJVl+ZLz33h464DtkAIHTGmFe1USKyWNFIYmCkvv1pa8v5ZLBnDpdVld30Yj1ZYARIc1ScAcYCwHBinMYUFepS4xeRViH7rGmCAhsBEU2OQPJCSJNtnJqxV8sKC8tLT9la7aQz3y4kdBQiTjyFNUpBzFp3hABXeDgewEEVQiZkoMDWWuPOgX9wNAb9/44dKDlk9s6EGRafyGnpZCJFI/RpA+2SCphtpJ76RLHQUQ2HCnEXqNx/sFCrJAIAsQYSCiU9LHxSp86IPw4z+YnJUsyiNTa2su72sqvtDfbSmxNG8HD7UTe5H/gFRPFNIvM1qlA0HiL4GEwAZB6eCwEG7aJxrHs2EXGdMvQH++9NnSg5z0A0XX31sGBEWEixXbw0LZoWxG3XMIhdLHuRxvAgzayDZ52IQlGq6QWQtAYj+PIChkLh4AAChCLUigQBwcwdUCISIqHTz74OZGSZthHeUFtbVOzVVJkhmyLMj0znb0weW/Ro8Bj9XEgtCiC/I13L/GkJt1x1a8UQDZzIPBgkY2TA7teuffK0IETYUDQ973Oo0jNleLiK0Lgoe6joZw9m4s91FF2BPbBlVsIGoSjcMNlodrVPNc1tAK9CoVLs4xopU8tEFGQUHqKmcYmIGmykZqaixySjH8eOioyDjKMZLJReVqcIo/V19fXaWEwqvR56dkGj0gkVX3AsoWxlwhIPVqNkY0TOeuDe++2hAOBbuwBEW4iGhCNRhQT8MgVYMbz2HCOC/kUL82R70aFEpynAQ5SUAg+C7YSQYWfrjq1QOiImgWRaipHYMcb86odfE6jhoZSYCQUSQwjiR5hgHNbByDrnGgkhuZyHm7yV9HqW3uMuzZLpwCS0x/ROGEDB9vufYAph7+EARm9fcjnddMSu04jnMt4LLmLNDTgcV5ElUJYB6lM9BrPEA0OiDYhmj+nFtIKUg2yqxGJsCCiM0388ximFggkJo2Y4sb6Cy4WZrUwPHCwoDOvpL28gK+3uymSJMNIMqP/sMix+ggIw8zZMykWjxu9J+LRh20CrXPgxWNVAII6MaJx4ujZgwXMA+zEnwAkjMibqNfdSmhRbySOKFvReeQib8rltlJ47sw53M6cQlABDUEHOAQRvncEgYNZFxIpNcBTviVLFFWEnfiTqFCeciA0i5dA/VpqbEFMdsA6lOpmhHhab+ftRjLXmakEYIK+CWZKCAhahl5m2YSiRFbIh4HIhFBegscSQI6tjGycUJJ1r+2jUM+bEUBuHELSQ+V67xXaJpTO5IrgQSWGeHSOasRzhXiE8j1aQIBZKInVLMRjVWhhqVQcYCLMJJREA4VTDESE9uQAg9wK3hBiUGOj7Thw8KEU0W7UjMQUZSAKs/SR0DpBMYkb8IFQp3qLYrIZ0UUA6YwAsh5A3kYVdDArEgiIvHnCRcUhaEzCpCMcyml6r5/WeIBI4RVYCgyFbxHCAGfQauCoO798wYLl43JhFUnAzzPOKQYi0l8Pg7yY0qguQiyhZgofgKChFGo3Up0UZSQiglBIJyHJIqbAIoOHhsPYNC6b2GYVSF/E6Hs+gLx2707XvgFMOUQCOZTlBmvzc70qEXE/OZQzOLy53FbOcH19JRc0IGISAWUhHMMKbioQqKxac+HCmh07FhgJmFUCEjoE2RRMpxoIua0U9ONpPKiJ1Nzc3HTxcrPopIiT4JiXjMQ/O5O/Q4wUSzOQVoQGRcytWxDiDRlcxG4d9bf6OJDW3ZGNk8ET7+Jv3NMyQGu4EUSweMh9FteV8dtEKqRjzk3lLDBAubgXwgsRSBxmx0LdfdUtX3N1zarlsC986VpoDdJSjXqN7ikFYvTjQ3xgS1NBga23jSO5U16bx8gdSeFz2ZmoQchAWvugF8lAeLpr8CAE4kZfAspLCOkCiL1hZXiS5WSK7wP8fW/1PPvx6N0IIKhFZOGzoIdS4WEjlywGXS1hIRoPHQsHQ2hWmK06HfDo7l51rk4ULbjpguNiCmWcCaaYjQE9PP11gggMBEIrpZSaWzARclvVyEKgkMdP7xCTfqEfMpDW1ta+VoSQSB7RdoL9FuTIAFJPQHLCY7rkePZwmwDClwwjkHx8XeZ5VpgmYXLpDMRLx6+5ztAWqXPq5tB5AMLym8vO1dGDOohnxwIL9iuYpFC5TtnWVAMR3d8AEakBEa6S3rbSAltzR2/vqdqmoqCTL5tL+ly2uFgcN5BWiIcQGf6qNRzFOgDqNLisY1twL4AcjQCCxZCPuwjIieyD1yKBiDY8asMwGmFUDCy82XtWPFKJiNtEWmgdH1WWLasThrIKWg4uBpNzCx1BcluBBEFkCoEY3V8QsapELpbYyru6Sgt6OxBWyEKwwW9pMxBYTOdrt0QDIYRwyUwhHvh2HBDHS4aNUHK8jrsuamP9rSoyyUIZAiBvnFA+0pcMjS78p4PZXgoik8gwlN5FHEEueOCRISDZACaRWBYuNMylbil4kFZdENoBKPBfEDcSjJ0QkTi8H6cYiNH9BZGNRIRcVomtt7y8vLe2aaOXeZ0Sho1hs3wGQjtrkb3YysWTXvFdn2CicdmEJNjQpi3inoD8T31kJwtlCIi82SMP05JhdFgfRstXJzA5FXTfoSvQWfThDW24XKh7sIkEG1kqsrAdFwyteXn5ecEEOwyeEJFkIjKlQIz0V3IGHUSkqbb8Dpq/aP2i92uVnLgUxEZRkjBx8kIiRfQtIKB5LHJfXAaNVgLCNo+zl3XcXFqp855DQAxVovn+ewDperNn2I8FkSif9V7PsNctIYg8RIsIx9e5ILMIZAwkZZzHGd2DReHQU7CXdftYsGDBf+24sGbVGbXXIohInMiUA6G/Io2ISHIeShGbrR0jje3NtRurJRoTmm9rsi2mwS1uJAnJiOjkorjWKcyrui8KKfo9txCU8poP26L6r7VEoApUDNkrQ1QXonUCS8iKBgKfdTjbi0qk4GFAci+dBZFFAHKWAxG7wuPtun1ADwj2ggcoLMRsHWnhDrCBmRCSM5gmVYlkTjEQY0AIbR0r6kP040tKNmKUPOjEE5hLQdnezBvAFEn45QBePNnaSrcX8VhhRMewEbEjIF6mhpZ1wlZaO3kFUh85k3Vg6I8A8jkqjoFoIDSAcn3YG7JWCj0PPffcc4sWLYo0kLMkAOGmohEpvHjxUlg8mZSJGj9W8WXeBRCwLODmco4jWWHm2W8ciEw1EDH6i3EUDMvxXqPVOiwF6WxdR14HT4DLm2y8AQzxC5ichAjIFiZzhxWtPgv1Gh0gAgl/tXs3dbGi128B5DUA+eLNnsHrtGQYoVGsHA5jHC8rm2RWlc0c1kqQMbjknuXCIxFLBJTTp7ATdAwoYVzCeKxZXodEa8ca/njHAk6EPFedIAIb4bnWVAMxLhaqOGW3WMkEGmtRra1cNFLacFaiRdQkALIZNAjJOrIP5zqgiZRIvyjW6OG+s2r3/pwJh+QGh17BX/LFn04MiiXD6NpwmAGvkFN2kmRviNMJOWqAhagIHrn06JKujtOChrFFYsFuOaVgy/mhP1O3/GXQ0LTjiDAblPB18FqCCH1m0NQDMan9eEimJX6ntRoBBVVJgd7bQgNYEc1IHOeTpBe5gQCOuo2TqODh3ICDbnBX9a24XsBE13QHEPQWYQgHdr0eDYRavm5qX3oV2eUaNHQA2+AwIyzM+vxzyzkQ7rGIiIjwi8SDSakQkCVLyH0RhjWXLvE4YmjNAg0PIVkR9HqR2sxEsmmaYiDiLBv048UM3UY6PZRajRd7S47rvS00gBXVRiwvEpAtzEkGAomYYkBRK3iFnBbUByBV9Z3IekmRdaELrSwBhC+IREX1Lz/KQvLgkrMHek4MjdeJHgzXDR444HKHYCuumueXa0AEkrOL+P1DoCwlKgTi6pKl2EcQMTLhl5fXraC5Ofh2ajVOORDRj9fODVXLdltpeUkJpoS4yldTo83N3/mIIw3rkPJq8QQ32nRTQXTxgp6REp8cyzl20o6IHg1EbgGQttuv9wzSSW3RwrCD4s3eNTB4cM9bb7z99h+F3n77jbf2HBxuAZcBGcaiEBTH84gqFNs5FNxHKxLKWTKQVReuXr2wdCmgiC0Ky4JVOwjJQh+ISKLVaJpaIEZBImttFAjrI+j6llwpL+3qaitdbHZibVMhG6FuCXIs1ISEIxwKYbEwOQsG4hUZMhnI2FpKslbaJwPyzSGfa4D3TqLbWQOugY9febcN3jNCXW3vvvb2Wx9nDw1d9w4OuryAwmAolwQTgvJwJstxt+YqKKwBFUEjmglKkyMLdlx4eYHDhzelelrm1AIxxlGcBpHaEgz/lhY0l5Q0X7bZcCWwEYmJbAs7CzXhQULgCLsHK9//2eLTq0YYSH1fJ0ZK10bN9WpA7sMzMT5MGqV9A4M9ewSBO+OlY/ngtd/vY0NDPkQWJ4KKZIXz4kj0bWIsuBWuWvV14ctXhS7oW7StILJTJrZj20A2uS0pzYTAa5pKIMY5JFobBbO/iOW9sI7S8g4bOo0Hgrvmr+brVoxWbN1qBmxAETfKhwc//O2/gpjCy8aTnTn9K5Fn4TInkwK5dvudHkqzbk1gIQDyFrGIshCDy50PXnvj4K6hAdegS3GHKMyrZjI5lVX4Kly15lLhEp0F7g0uXGGmsorWsLa7WpjC0G2ND7/oFh7HGggk5uNFGwVqKm8rP267jI52Owr34V0fLjplq6XT25m6TLi54eQEanUw+d9/8VOst4PZOnJj9npkvrjiyaQW0nUN6S0MIdpCvkEMGeTtrsmksep65fcHuZ3IxMRZs+Cs0MRUXl4CC1lyYVXh18uu6rqg3chGwiSsZA0RepW1hBS3x5MUTwgyVZnwMPZAqCDBhC6fMyXZCsrLT9HJiNX+oPnDN0ppSt62mNwWScE6CIg0YNOke6zffPbTdYMobKhqwartyr7WtRgBWtkfDcTVQ0BGyRJkRrV6dJY1yLsr0EOggMkbHw3tcg8OKyOSOaSayURMCl9+uRB3a7iBhMlA8s7291/dtm3bq+9v3w4YOhVCMtLiY16PBx8+YVhIehrOrYk5EHFpKEmmK7fQyBa1UfKK6Kx63/Pvivz3sm2j6rbIUjZzHuFUNjHXv/3us5+uRa7FvVpnlb3vZN/K/pUYkosO6qIOuYtJRcqzbo9G1SHZyuAu1I58e7ihtL32lm9oABFeUWAmK3g0CbcUerjkwhJQWXp1aeElMhBDO9T7d7a5fAOQ2GUPH972/naNCpC8fzirZSDLH2Cz4tLiU1PxkRiJEg3UxRiIUZAo3qBoo9DIFuSo3thULv7PMJImuC1BxM2NpMGAgj2qwuF//wWAoBqBEcFC6nM6sbPTpBzJHgHkNfxeWAZmTAavI4pEtXtdjH0QefCxTcrkg7cPDvW4XF70gsyS5rnCoVxdxvdXzwLKeK1aJYxkm28g242cEqIdCz0LLsy191XDWN7f68rOYnSBFFIgNGcqXJZRkLj1q39hsK8mj2ZOS/SRlFpbnoyKBPI6GQIJOOhUyECGfQdeABCQoUSs9Wg94krDWM5+dfK9ym4AkWThj764dR9n36KPeAhEwoD8CU2ufW1dbVxd0J0HchGu67U9PSfkQa9bdsNzaUgMLEuvLrlELP7fJTKQ7qvdawSPNcsFj8MtXkpvDbkJDMuCvWS79pKxcG3fxuhCHCHJ48fcbeyDukEE/XhRaWM+CIU7rxSbLtps5eQ07vC5LTSDuXAHIyEaKhREkGF4rHFAdq9sPdmXM3ZS7S72r6zv15uLijQ49BYO4RfX7uKckL3X3a5Pbt8fjUiyMGR6bfQWNDo6evfujdtffPH5520gY2CJZvLuG74h96DCRuC5gCTcTLqX0X5Z9xKKIERkFXZ4sFwYyN6WETaROJVs30DLQLZ/+DA0zCQqFP1J2qiUiRRrIPitvCCBUwIKvW7H2lVvyWVMNpIK5gOIomhG8iKRgAgKst0ReCwDCJ4jt1VPia8AMi62V4Ycvo8w5/I5asJbNz79+EQ2e/3+7dFb41pZw1mHxy0ljo4CzTVo9O5t4qJiCWciPNfv/UMhIFHkcCQwkO6llxBHgOXSsu5uGMjLC7qJyLLlHM/2KB7RxsKys3y+Z7P9noAnO45wkFQUJvFNzICIFRLmlxTJKNohW1NpKZKsK8i8Ck5ZJcXJVxL1Vgpg0BcFjpbffPbnz/6VgGD9SmTClGidVDsnRn3YjwUqp4uietstfvivvTnQ07Pv0I3bd0dH1SmH68quqC7wXWxAAzC3gAVUJmQCJN6hbBVJzXKDCExDYClc0s21fFU3oVi69Go3to98XvYw4VQSL2CIK87qEmMHqsAkRkBEQZLtCcna7ANUixQYzd87pQUduBAEDARnxSHb0ozEASMh0fT18Ie/1YCg29WwGzbSVzXWAChCdpDReMDQUGVQmjVKx//ujU/2ZP9haN97X96+ISxkzx9cryPiTyLicgtUdCigYiARVpIlkIQMJDeXcSw3lxQu5TxePgJDgaUsXUYGciHbzR5B/lmzU9PnqpcPy5gTnzY7LiEpcVZiYmJSQnJaPF4iJjEBYhQk4lTEZlDh51CXdrTTyW8F7XlMBqwSUSQaRgLRUmHW8AsA8pOTMBZ8PnFOTisMx17fYExk7YbvEjzosjOKmyblENW5Q7rxzaG/7Btu2fvmoftch977FGgeKFABlPu3CUo0kreHYSVe+Fcze17wWPLtUrr79leAIgzkHL9bRnjgsQYU9kiaGYdP6ctIxYePzwz51cueIaBgj4f0agY5mxgBMYkBIX7im2YiTTjTqpcuk1KNFuSz8y+in5KnuS2UgBYYCRiw4Q9/Bx4viG8srfUrWxtac/qrGhqqNEdVtdv4xGEIJiKCCAnp7+37X3566NCn/Ft8h/jxSIIDg6WImIKdjqQNVqIM0jy42bFAAFnC95rHWla3qlvX1Zvv60AkbPpdtPz0KX0h7D1+9ackCjFathwIZMfRyGcsgBgFCUpELFVBzdgwilJbm1fDgjixZ3V5Fy6maRO9LRIlwC8yeCy3jwzkP082bGEOtnn32O6TDa31/TkNR/Xmu/1v9QYPZYS5UPh1gYcQosc3N27c+Eb77hF5wFLIfY3CUO6EI4Hj8u0adjG3GkqW/kpg0TzWqjoYii4NiMQoYnv82OOIT8hEAhS8IqnBXuEFi29AVZbbi8n5dPit2AAhY0v2MJp9oDWrZmg1P9EKOKx56kXp9N6W6DeCBxv+JQzkz0iyyH2xdTn9cFm4lnXO0a+MgrA+R7tMFvr0AyOy70AbVSKQTgEcHkd3yXvd0JgYSN7o6RkcBnzyW0teJyBL/xdfOP7d3WeWd09gIR7PMwm8Co9PS06ii2Ey6UGJlzsbJHxs+DDKFAhdl8M+n1t8GIUpBkD0FRKJzz5IXovFalHEmSQ1ec22AlEW3xG9LWHjbhn5oFtpgYFQTG+wMHgsDmS3fazqq69gFjqReuKB4Dk8sPfj68Ounn13uuCzYiIYisFER/LuW7taBmXyW9ZVSzkQYFkGHvBY4GLoJmIIjc8mxSMsa8qIT57pB5MJWCichVEwGj2x7XsHQm6mTXPNiN1n2ip0/X4JMCBmrd7IV9tPt6mXSTmO3pZKBNW7MJDPPvvsJw0/5xMqOf31JwlI/d/+lhPWxeKubiSbHbq297oL1eGdL4hIDJnc/rwrDMkr+4Z8B2DBWdLzHAiw3IS6l54BF9pUIO/4mM/LP24k/IrK6SlJEXWiMuINEYvBvYRCB4EvNIap+N+O3ySmgmMCRHxGDxUT2ElKjX5eD9Kuy3rZTpdJCTqNdwwiCIWQhqMvUghpyOm3H22gy8Lm/I1PkRolugwezPfJbVytt8d1YOitz2/AacVM5Lvuf2GYCf1j//gRinemeAesyxFDlpxdcpO0BHfd2gYgV92D21pGpOSMiM83TombaRiGMqKwZwdafMO8vXUVrs4QuvRXfwxRF/IwEUmOGZA0GAitjdNSOyiQmqPL9tJqHQhSrP/733/+jELI0c3MgRJ+N1CcbAAK+//89a9GF6sy6KC3GDUTR1GgDykHhvZ8eRuBI7ZM4Lq6xiFpe9s35KKz5EQGLICcXcrvuvVt28jVV3u8I6G4eLrQdebcjPT42QnPUE7FWZBhZKN9wg5v4ywEDAPH1R//7D9e+MXvfnNg2HX4/e2Hs90SnVkdAyDaNX+2bMp2eiV1FVEvEzu6SpEC99I1ORYxUBNSqK34GS8Lj9JUyr8cbajvr284Sd7KnvPXv/6VRnu1gO4ePoHBH1Ggt/QcaDn8ye27MJKYapTMhGjo0f0EorvCzDU6kCXLburqptuFlgs33/cNeEcCbvOsWbNmZjOkWzyb8pJh+GAYrr3vvwMWBgwDx8/+Awfgdy2/bGGMPdsychh3/FoXM2L0mc/O7H85CiJykHEiWpV4saOW1nYLjqMs+VAyDOSXv4WBgMhPjjZYgOnnR0/Spd5bgYWQ1OdA9eqVeplLzP2IAn1fy/UB359uUWcxtoKZwHMZSN7dg+USRTaPLNCA/ErHIfbfvr/925vowF9/1j0yMiKuueHlLLI0wwALQaNb34R+Ru/HF37zoTKsuCEFw7cSjaDGBkhKAB1Gx8+/Orol2+lEQWLYiK29tLS3g64027wYQ/J0AgWJ2rzggRCy9ujPqVvcIOLHylaEdVV2O04sDDmJxzvf4PBrBfon73x0/Q8fvXcDdXnMkVy78bmB5M5rB4cw5yXBSF6/efPbm9xjhTO5Sju+CtUy4PNlZWX7fD7BAobRTWZEAhWdhkDy4//87Ne//vMLv/k3l3qWe2JaAgv4JbqqaSyAxPHPLfrq6FdHN8NGWLCo2RjZwumIl2lxtybIUCZaQQ4xGg4LBiI8Fo1iWxqOHqWOiX13627Rczccluv6x9f0Ihxgbt/48r0/fTzyzqH7sdfd0Wt3KZioIb7rbd8J+C2z9c1vv715k1OZSN92X3jn/W17Dx92HUZ18ep2clLELEIEQyD59f+n7vxjmzrTfK/+oBTaaQsdZLmeqkLAHwgQSIO0C6jSTJI/UoVKV4quItGUDZOb7TZd1FKSAEOCMg2IH2EUJUpufiBBfm1BMcwmDR3ZrFZNlX82qNpo5Ei30mDHdkh85tiOk+PA2Fg93O/zvufkTXychuTYDvuYTTzT2ZT4c57fz/u8N8q27rpxI79Ymk/mX3lz0/rEo8T0y6sAstlwUdxL07BYxwDk22+P2TzYmomz7Ny1s7T9z//7QxxTDf398m/+hfGIKMV7wINk2234dA0IPDoRoWsqxK532rEohn64lozOzo7ae3riTzOKhLuSM6g5ehK2r4jIj9oLkuRNfqR/zD5tzZqllN/u2kVIoB6lW3PgPw6Chy6PXsCdzBvnpt/ZBCAmiIi7CG3vAwiQoCrioRTx8r+yntVvfvPhP6HVvgUz8h/++UNmshRUecGDWazbt+HTORAyWuTRkY38MwdCJZOQ4m0XvUFRJRllVd5MItFcyReNEz5Jddb+5Tt87PhjwKJHXuItxIgEJHaV0VeoRumBshs3DjcIHnTMCUR++foLj9atGAiynyQgr9DGV9s+6AcR2Qc6kQdgAgEI/gX7H/5MZ0MpI1Q5D81i3b5MQ8C3SSgvRPD7xz8CyR9o4t0TggM5pfEwQsmMxBEGAwlY8D/YKhj1KV7pv7+DHnAoqZgIxyJi48U8cm5s/Su+wpfnoEaxF7HY4nVbG9jkyIYVAsHWvsU3ZGxGVsgGS3aDBpN9NInILg94oIqsnY6G0n8tF25hPHSLtRsWjDSEhNcV/3AVSG5f/Qe6T0eiOd6sSxKSLzonOmVrFGYLotNYWle4FaPvi7Dk9Of8FV/xGDLvWVQoL64V06ZpFGHWrxAIGSjMTSTFWLAsAKLL7ss2Ft9G/n6Zeu2/4TPyjIdsayhi/nzeYu1D5xARGgNCnp0uXPtnQvI+eMjebtqanHWJk+EK8LyEZgPOoC4Ps/U3UhJBJTUUIx5mwUr7YbDAQ7MNew5yHthcCZnmTN5aRaZOn/+LfHJCWCwNiCCChIROLHyoe3bIvzIexVAPxqNft1jv2x6EIkgMIVxJyFqhzHv7I1YWpuGStREgYakiU5JPnE5f0Nn7V05EMEmpKgKG/o143Niq6wdJvjOoOfOX169/+aU5xLxsR+DmlQPZiOFI7I8Qm2BfmFMZkIfzQJCQWDEeuuVDHvsKHoX5e6tOngQOjgQW61sAUT0AoguzVn/4w//9X+wCsBPgsUYSZ6kiC7goTeye8MlRtZqICGFIlvb1gk1pf/8uslqcx8n+ww0yD3ffeJNfh7ox8YiryEqB0BpFrS4pdsxAPAkAWYAERCKKWMXxL5xH7smqk/39QLH3pGaxKA3hMbOQP0JIQZgDWVPhroTXRr+ckKWE86sfdSRGLktTAY/Sv/xlVz89jKV7hYLMrdMvEKWYFwdKVgyEr8wQp0z5ThNeOQEOINGoPERCovAzJODxG26vcm/0nwSO/v6dO8lw7b8KDUGiTuX320kCxQl6jSsbsu/d4Up0s+Wy+EJ+ciT0ErLYq6RgAntV07z117uo/dO/a5euIOz4NGDwgSDsB331EW02XikQVNrZ1huUN0WlF0Csx6AhDxkR/nrfpnrYiQUwebBFYem5zuP/0FfUsSCUh2Dvxu4kIFAQj5NnIGuMJOzu0wrBnwX9PsXZ+xdGIyUWPf5ajCWnqqt/16+3nrzRfOPG1q03ILnFpCCLLtwkJBvpIt0VAYGBWkdBLjrzr9L6NN6b4hryu4caCV1HkJB42IkFJCDMYO2BvSLZ+yumIOUIk6EiCLNClFYmK4hUTyXFNRcEXLBb3Gxd6Z7oDEbnkJFoPJa0X4LIj4d2VbVW5RyC2cLzuPW/6aHcUygbNmHzSdA3Vgxk4zsAokRVz/QLo... [truncated] |
| ✅ With the latest revision this PR passed the C/C++ code formatter. |
| | ||
| // First priority: Check if partner is already allocated | ||
| if (Partner.isVirtual() && VRM && VRM->hasPhys(Partner)) { | ||
| MCPhysReg PartnerPhys = VRM->getPhys(Partner); |
There was a problem hiding this comment.
getPhy doesn't return MCPhysReg. It returns MCRegister. There's currently an implicit conversion operator from MCRegister to unsigned, but it is supposed to be removed in the future.
There was a problem hiding this comment.
then how do we know if the register is already allocated lol?
There was a problem hiding this comment.
This isn't a question of allocated or not.
MCPhysReg is a typedef for uint16_t. MCRegister is a class. It contains an implicit conversion operation to unsigned that makes this assignment work, but that operator is supposed to be removed.
You should use MCRegister instead of MCPhysReg so you don't rely on the implicit conversion.
Once you cahnge ParterPhys to MCRegister, you should use ParterPhys.id() for the addition on line 869 to do an explicit conversion instead of an implicit conversion.
There was a problem hiding this comment.
Oh I misunderstand, I was thinking that getPhy is getting removed
| Great to see someone is contributing this work, as we have looked forward to having it too. I have a few high-level comments, which I don't think are covered by Craig's review, and might supersede his comments: Is this based on the NXP Patches against LLVM 18? If so, this will need appropriate Co-authored-by. Even if it isn't, I will add @anmolparalkar-nxp as a reviewer as he is familiar with that work. We already have one pass that does this optimisation: I am glad to see the new |
They aren't really doing the same thing are they? This pass uncombines the pseudo instructions that didn't register allocate correctly. The existing pass combines small loads into wider loads. |
Wouldn't modeling it that way imply that FLD and FSD would support 4 byte alignment when Zilsd supports 4 byte alignment? |
Fair question, but my feeling (no hard data to back this up) is that cores with the 4-byte Zilsd support would likely do the same kind of thing for D accesses. I would especially expect this for Zdinx cores. If we do make it an unaligned scalar mode, I would probably rename it Maybe this is too much and a zilsd-only option is good enough for the moment. |
They aren't, but I think it would be good to have only one early pairing-related pass and one late pairing-related pass, especially if we're going to run the late passes at such a similar time. I realise this PR isn't the one that's going to make the existing late pairing pass support Zilsd. |
I agree with this. |
Furthermore, if it cannot be embedded into |
No I started this from scratch, I didn't even notice there's existing one lol |
I feel like mentioning https://reviews.llvm.org/D144002 ( |
| Fixed all comments including moving post-ra fixup pass to RISCVLoadStoreOptimizer |
lenary left a comment
lenary left a comment There was a problem hiding this comment.
Some comments about edge cases.
| // register This prevents register reuse issues where the first load | ||
| // overwrites the base | ||
| if (Opcode == RISCV::LW) { | ||
| if (FirstReg == BaseReg || SecondReg == BaseReg) |
There was a problem hiding this comment.
I also don't believe that the Zilsd specification, as ratified, includes this restriction. The 20250508 version of the unprivileged spec does not mention it. I think this did once exist in an unratified Zilsd spec, but was removed.
| if (OffsetOp.getTargetFlags() & RISCVII::MO_LO) | ||
| if (OffsetOp.isGlobal() || OffsetOp.isCPI() || OffsetOp.isBlockAddress() || | ||
| OffsetOp.isSymbol()) | ||
| return OffsetOp.getOffset(); |
There was a problem hiding this comment.
I am still not sure this is right.
If there is the MI equivalent of lw a0, %lo(sym+8)(a1), I think the getOffset() is 8. This doesn't mean that %lo(sym+4) is necessarily 8 (it depends on sym as well, which only the linker knows). We can't merge lw a0, %lo(sym+8)(a2) with lw a1, 12(a2) (this may be impossible to get in the compiler, because both should have %lo or neither), but maybe we can merge lw a0, %lo(sym+8)(a2) with lw a1, %lo(sym+12)(a2).
Maybe what is confusing the situation is that you're representing "couldn't understand an offset" with returning 0, when 0 is a valid (and not unlikely) offset.
I think this function would be clearer with a boolean return value for whether this understood an offset, and an out-parameter of the offset that was found? You might need another out parameter for the thing the offset is relative to in the %lo case.
There was a problem hiding this comment.
Oh I think this function is unused and can be removed.
I think what you mean is getMemoryOpOffset in pre-ra phase. Before getting offset, it will check if symbol type is same in 2ff058f#diff-d0a753ec0b19f66e691696118a7297232914a41f21e0bd452a3cde1ef3a3d2adR196 and its flag in following lines, so I think this is safe lol
There was a problem hiding this comment.
Ah, I see. You are right that this one is not used/necessary.
lenary left a comment
lenary left a comment There was a problem hiding this comment.
I did a bit of a closer look at the code this time around.
I think there's still some issue where we treat symbolic offsets similarly to immediate offsets. This might appear with more testing where both an in-parameter and a global were loaded/stored to/from in the same basic block, especially if those loads/stores were interleaved.
Hopefully I have also proposed some simplifications too :)
| DebugLoc DL = Op0->getDebugLoc(); | ||
| | ||
| if (IsLoad) { | ||
| MIB = BuildMI(*MBB, InsertPos, DL, TII->get(NewOpc)) |
There was a problem hiding this comment.
Does NewOpc need to come out of canFormLdSdPair? There's only one opcode for load, and one for store, and you're already in an if (IsLoad).
There was a problem hiding this comment.
That's right, I think it doesn't need to be output of canFormLdSdPair
| const MachineOperand &OffsetOp = (Offset == getMemoryOpOffset(*Op0)) | ||
| ? Op0->getOperand(2) | ||
| : Op1->getOperand(2); |
There was a problem hiding this comment.
Instead of re-looking at Op0 and Op1, why not have Offset in canFormLdStPair be an out-param of type MCOperand& (representing the lower of the two original offsets)? This should simplify this code quite a lot, I think, so you wouldn't have to keep checking what kind of operand it was.
There was a problem hiding this comment.
yeah thats correct, and I think we can just use add(OffsetOp) instead of reconstructing operand~
| llvm::sort(Ops.begin(), Ops.end(), [this](MachineInstr *A, MachineInstr *B) { | ||
| return getMemoryOpOffset(*A) < getMemoryOpOffset(*B); | ||
| }); |
There was a problem hiding this comment.
This is quite suspicious, as this will do odd things if the BB contains a mix of four pairable loads with their 4th operand the following: %lo(global), %lo(global+4), 0 (immediate offset), 4 (immediate offset).
I'm not sure what the solution is to this, except a good deal more complex sort function.
There was a problem hiding this comment.
Oh that's correct, I think we should also sort symbol so that same symbols can be adjacent, thanks for reminding!
| | ||
| // When no memory operands are present, conservatively assume unaligned, | ||
| // volatile, unfoldable. | ||
| if (!MI.hasOneMemOperand()) |
There was a problem hiding this comment.
If the base instruction has two memoperands, this will fail. Maybe that's ok, but it does seem a bit odd.
There was a problem hiding this comment.
curious does RISCV have any instruction that have more than 1 memoperand?
There was a problem hiding this comment.
I think it can happen if we merge two separate instructions with different memoperands. I think this case is rare so this is ok.
| Align RequiredAlign = STI->enableUnalignedScalarMem() ? Align(1) | ||
| : STI->allowZilsd4ByteAlign() ? Align(4) | ||
| : Align(8); | ||
| if (MMO->getAlign() < RequiredAlign) |
There was a problem hiding this comment.
MMO->getAlign() takes into account the offset when computing the new alignment. When we have global (align 8) + offset=4, that align is 4, not 8, but we should be able to merge it with global (align 8) + offset=0.
Maybe we want to:
- check only the MMO's base alignment here. If that isn't large enough, then the optimisation can never happen.
- check the base+offset alignment once we know which the smaller offset is?
There was a problem hiding this comment.
That's a good point thanks!
There was a problem hiding this comment.
btw can we do optimization on 4 byte base alignment + 4 offset also? e.g. global (align 4) + offset=4
| // For symbols, must be the same symbol name | ||
| else if (OffsetOp0.isSymbol()) { | ||
| if (!OffsetOp1.isSymbol() || | ||
| strcmp(OffsetOp0.getSymbolName(), OffsetOp1.getSymbolName()) != 0) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
External Symbols always have offset 0, so there's probably no point in accepting two loads/stores with external symbol operands in this pass.
There was a problem hiding this comment.
uh I see, let me remove it, thanks!
| } else if (OffsetOp.isSymbol()) { | ||
| MIB1.addExternalSymbol(OffsetOp.getSymbolName(), | ||
| OffsetOp.getTargetFlags()); | ||
| MIB2.addExternalSymbol(OffsetOp.getSymbolName(), | ||
| OffsetOp.getTargetFlags()); |
There was a problem hiding this comment.
I don't think we can get here, as the offset of an external symbol is always 0.
There was a problem hiding this comment.
@4vtomat this was not addressed. And if it can happen, it would be incorrect. As now we would have 2 4-byte loads/stores accessing the same memory. We would need an offset for the second access to maintain the behavior of the paired instruction.
There was a problem hiding this comment.
sorry about that, I just forgot to push some fixes and I thought I did..
I think it can be removed lol
| // Add offset operand - preserve symbolic references | ||
| if (OffsetOp.isImm()) | ||
| MIB.addImm(BaseOffset); | ||
| else if (OffsetOp.isGlobal()) | ||
| MIB.addGlobalAddress(OffsetOp.getGlobal(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isCPI()) | ||
| MIB.addConstantPoolIndex(OffsetOp.getIndex(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isSymbol()) | ||
| MIB.addExternalSymbol(OffsetOp.getSymbolName(), OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isBlockAddress()) | ||
| MIB.addBlockAddress(OffsetOp.getBlockAddress(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); |
There was a problem hiding this comment.
Why are you bothering? Just use MIB.add(OffsetOp) I think.
| if (OffsetOp.getTargetFlags() & RISCVII::MO_LO) | ||
| if (OffsetOp.isGlobal() || OffsetOp.isCPI() || OffsetOp.isBlockAddress() || | ||
| OffsetOp.isSymbol()) | ||
| return OffsetOp.getOffset(); |
There was a problem hiding this comment.
Ah, I see. You are right that this one is not used/necessary.
| Before I do another review of this code, please can you once again go through the comments I've left and check that you've taken them into account? Even the ones you have marked resolved? Going forwards, please can you leave reviewers to mark their own comments as resolved - you've marked some of mine as resolved without actually fixing the underlying issues to my satisfaction. I don't know if this is because you made changes but had problems with git, or because I haven't been clear enough with where I see the problems, or something else. |
I've gone through comments again and make sure it solves your comments and leave some comments on those cases that I'm not sure. The main concern is different symbol can't be fold and I also added a test case for interleaved symbol.
Sorry about that, I click resolve when I push the fixup commits, I should leave reviewers to mark as resolved themselves. |
| Gentle ping~ |
| } else if (OffsetOp.isSymbol()) { | ||
| MIB1.addExternalSymbol(OffsetOp.getSymbolName(), | ||
| OffsetOp.getTargetFlags()); | ||
| MIB2.addExternalSymbol(OffsetOp.getSymbolName(), | ||
| OffsetOp.getTargetFlags()); |
There was a problem hiding this comment.
@4vtomat this was not addressed. And if it can happen, it would be incorrect. As now we would have 2 4-byte loads/stores accessing the same memory. We would need an offset for the second access to maintain the behavior of the paired instruction.
| // Add offset operand - preserve symbolic references | ||
| if (OffsetOp.isImm()) | ||
| MIB.addImm(BaseOffset); | ||
| else if (OffsetOp.isGlobal()) | ||
| MIB.addGlobalAddress(OffsetOp.getGlobal(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isCPI()) | ||
| MIB.addConstantPoolIndex(OffsetOp.getIndex(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isSymbol()) | ||
| MIB.addExternalSymbol(OffsetOp.getSymbolName(), OffsetOp.getTargetFlags()); | ||
| else if (OffsetOp.isBlockAddress()) | ||
| MIB.addBlockAddress(OffsetOp.getBlockAddress(), BaseOffset, | ||
| OffsetOp.getTargetFlags()); |
| // Create two separate instructions | ||
| if (IsLoad) { | ||
| auto MIB1 = BuildMI(MBB, MBBI, DL, TII->get(Opc)) | ||
| .addReg(FirstReg, RegState::Define) |
There was a problem hiding this comment.
Is it possible for BaseReg to be the same as FirstReg? If that happens the next instruction will get the wrong value for its base.
There was a problem hiding this comment.
Test case that miscompiles with -mattr=+zilsd,+unaligned-scalar-mem
declare void @bar(i32, i32, i32) define void @foo(ptr %x) { %a = load i32, ptr %x, align 4 %b = getelementptr i32, ptr %x, i32 1 %c = load i32, ptr %b, align 4 call void @bar(i32 %a, i32 1, i32 %c) ret void } It produces
lw a0, 0(a0) lw a2, 4(a0) The second load is using the result of the first as a pointer.
There was a problem hiding this comment.
You are right, it's possible to be same, I just use early clobber to resolve this but not sure it's too aggressive?
There was a problem hiding this comment.
I think the other choice is to just emit the two loads in the opposite order. i.e. for the case above,
lw a2, 4(a0) lw a0, 2(a0) Would be acceptable, and would prevent needing the earlyclobber. This could be a follow-up change though.
| | ||
| auto MIB2 = BuildMI(MBB, MBBI, DL, TII->get(Opc)) | ||
| .addReg(SecondReg, RegState::Define) | ||
| .addReg(BaseReg); |
There was a problem hiding this comment.
We should preserve the Kill flag for base register.
| LLVM_DEBUG(dbgs() << "Split LD back to two LW instructions\n"); | ||
| } else { | ||
| auto MIB1 = | ||
| BuildMI(MBB, MBBI, DL, TII->get(Opc)).addReg(FirstReg).addReg(BaseReg); |
There was a problem hiding this comment.
We should preserve the kill flags for the registers. Need to be careful if FirstReg and SecondReg are the same register.
There was a problem hiding this comment.
I think we already ensure FirstReg and SecondReg is not same register in pre-ra pass, I think there's no something like CSE happened between pre-ra pass and post-ra pass?
| OffsetOp.getTargetFlags()); | ||
| | ||
| // Copy memory operands if the original instruction had them | ||
| if (MI->memoperands_begin() != MI->memoperands_end()) |
There was a problem hiding this comment.
Can we use memoperands_empty()? Though I'm not sure why we need this check at all. Doesn't cloneMemRefs work even if there are no memory operands?
There was a problem hiding this comment.
I think we can just remove the check lol
| // Copy memory operands if the original instruction had them | ||
| // FIXME: This is overly conservative; the new instruction accesses 4 bytes, | ||
| // not 8. | ||
| if (MI->memoperands_begin() != MI->memoperands_end()) { |
There was a problem hiding this comment.
Can we use memoperands_empty()? Though I'm not sure why we need this check at all. Doesn't cloneMemRefs work even if there are no memory operands?
| // Copy memory operands if the original instruction had them | ||
| // FIXME: This is overly conservative; the new instruction accesses 4 bytes, | ||
| // not 8. | ||
| if (MI->memoperands_begin() != MI->memoperands_end()) { |
There was a problem hiding this comment.
Can we use memoperands_empty()? Though I'm not sure why we need this check at all. Doesn't cloneMemRefs work even if there are no memory operands?
| MachineInstr *Op1 = Ops[i + 1]; | ||
| | ||
| // Skip if either instruction was already processed | ||
| if (!Op0->getParent() || !Op1->getParent()) |
There was a problem hiding this comment.
What makes an already processed instruction have a null parent? Wouldn't the instruction have been deleted rather than just having a null parent?
There was a problem hiding this comment.
correct, I think at this point it's not even removed lol
| // Special case: First register can not be zero | ||
| // zeros |
There was a problem hiding this comment.
| // Special case: First register can not be zero | |
| // zeros | |
| // Special case: First register can not be zero |
| auto MIB1 = BuildMI(MBB, MBBI, DL, TII->get(Opc)) | ||
| .addReg(FirstReg, RegState::Define) |
There was a problem hiding this comment.
You can pass FirstReg as a 5th argument to BuildMI instead of explicitly using .addReg(FirstReg, RegState::Define)
| // Special case: First register can not be zero | ||
| // zeros | ||
| if (First == RISCV::X0) | ||
| return true; |
There was a problem hiding this comment.
Don't we need to check that Second is also X0?
There was a problem hiding this comment.
The code here isn't great, but this does now do the correct thing.
if (First == X0) return (Second == x0); Would be equivalent to these two initial ifs and simpler.
| //===----------------------------------------------------------------------===// | ||
| | ||
| bool RISCVLoadStoreOpt::isConsecutiveRegPair(Register First, Register Second) { | ||
| // Special case: First register can not be zero |
There was a problem hiding this comment.
I'm not sure this comment matches the code.
| ; WITH-HINT-LABEL: name: test_load_pair_hints | ||
| ; WITH-HINT: liveins: $x10 | ||
| ; WITH-HINT-NEXT: {{ $}} | ||
| ; WITH-HINT-NEXT: renamable $x11 = LW renamable $x10, 0 :: (load (s32) from %ir.p) |
There was a problem hiding this comment.
Is this output identical to the WITHOUT-HINT output? Looks like the alignment check is failing?
There was a problem hiding this comment.
yeah thats right, fixed with 4 byte alignment check
| define i32 @basic_load_combine_8_byte_aligned(ptr %0) { | ||
| %2 = load i32, ptr %0, align 8 | ||
| %3 = getelementptr inbounds i32, ptr %0, i32 1 | ||
| %4 = load i32, ptr %3, align 8 |
There was a problem hiding this comment.
It impossible for this load to be align 8 if the previous load 4 bytes before it is also align 8.
| @lenary @topperc My apologize, I usually use 2 laptop to work so there's some part of fix that didn't be pushed and I thought I did..
|
Thanks for explaining, this makes sense for what happened - I'm glad we found out what happened, as you are normally very good with review comments. |
lenary left a comment
lenary left a comment There was a problem hiding this comment.
I just did a close reading, and I think I'm happy enough? The below are broadly just nits. I don't think any of them are required to land this.
| //===----------------------------------------------------------------------===// | ||
| // Pre-allocation Zilsd optimization pass | ||
| //===----------------------------------------------------------------------===// | ||
| class RISCVPreAllocZilsdOpt : public MachineFunctionPass { |
There was a problem hiding this comment.
Do we need an override of MachineFunctionProperties getRequiredProperties() const? I think this needs IsSSA but I'm not sure.
| | ||
| // When no memory operands are present, conservatively assume unaligned, | ||
| // volatile, unfoldable. | ||
| if (!MI.hasOneMemOperand()) |
There was a problem hiding this comment.
I think it can happen if we merge two separate instructions with different memoperands. I think this case is rare so this is ok.
| unsigned Alignment = MMO->getBaseAlign().value() + Offset0; | ||
| if (Alignment < RequiredAlign.value() || | ||
| (Alignment % RequiredAlign.value()) != 0) |
There was a problem hiding this comment.
If we know MI0 is the instruction with the lower offset (you sorted them by offset, so I believe this to be true), then we can use MachineMemOp::getAlign() on the op from the lower instruction, and compare the two Aligns directly, because the lower of the two offsets needs to be the correctly aligned one
| unsigned Alignment = MMO->getBaseAlign().value() + Offset0; | |
| if (Alignment < RequiredAlign.value() || | |
| (Alignment % RequiredAlign.value()) != 0) | |
| if (MMO->getAlign() < RequiredAlign) |
S
| // Special case: First register can not be zero | ||
| // zeros | ||
| if (First == RISCV::X0) | ||
| return true; |
There was a problem hiding this comment.
The code here isn't great, but this does now do the correct thing.
if (First == X0) return (Second == x0); Would be equivalent to these two initial ifs and simpler.
6 participants
This commit implements a complete load/store optimization pass for the
RISC-V Zilsd extension, which combines pairs of 32-bit load/store
instructions into single 64-bit LD/SD instructions when possible.
Default alignment is 8, it also provide zilsd-4byte-align feature for
looser condition.
Related work: https://reviews.llvm.org/D144002