I’m looking into problems with the function DAGCombiner::MatchBSwapHWord and had a couple questions on how to proceed (I’m new here, and to llvm). It looks like there was never any test coverage for this code, and it doesn’t match the patterns it claims to be looking for. I found a fix for this in our fork, but the fix is even worse in that it matches a whole bunch of invalid patterns as well as the canonical patterns. I have simple test cases that break both the existing code and our local fix. I see equivalent matching code upstream in InstCombiner::MatchBSwap() (InstCombineAndOrXor.cpp), but not an easy way to re-use it AFAICT. Questions: --Is this functionality even worth fixing in DAGCombiner (ie, what is the probability that this complex pattern would re-emerge in codegen after the InstCombiner transform, weighed against the obviously error-prone nature of the matching code)? --If so, I can patch up and test the existing logic, but since the original code is quite old I wanted to reach out for any modernization advice before embarking on that effort. Thanks, Jim -- The information contained in this message is intended only for the recipient, and may be a confidential attorney-client communication or may otherwise be privileged and confidential and protected from disclosure. If the reader of this message is not the intended recipient, or an employee or agent responsible for delivering this message to the intended recipient, any dissemination or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify us by replying to the message and then deleting it from your computer. Please note that KnuEdge, Inc. reserves the right to monitor and review the content of any electronic message or information sent to or from KnuEdge employee e-mail addresses without informing the sender or recipient of the message. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161208/26ecf336/attachment.html>
On 12/8/2016 10:01 AM, Jim Lewis via llvm-dev wrote:> > I’m looking into problems with the function > DAGCombiner::MatchBSwapHWord and had a couple questions on how to > proceed (I’m new here, and to llvm). It looks like there was never any > test coverage for this code, and it doesn’t match the patterns it > claims to be looking for. I found a fix for this in our fork, but the > fix is even worse in that it matches a whole bunch of invalid patterns > as well as the canonical patterns. I have simple test cases that break > both the existing code and our local fix. >Are you sure there isn't any test coverage? As far as I can tell, the tests from https://reviews.llvm.org/rL133503 are still in the tree.> I see equivalent matching code upstream in InstCombiner::MatchBSwap() > (InstCombineAndOrXor.cpp), but not an easy way to re-use it AFAICT. > Questions: > > --Is this functionality even worth fixing in DAGCombiner (ie, what is > the probability that this complex pattern would re-emerge in codegen > after the InstCombiner transform, weighed against the obviously > error-prone nature of the matching code)? >I don't think there is any existing code to match this particular pattern in instcombine. I guess it wouldn't be hard to add, though. -Eli -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161208/83819c36/attachment.html>
>> Are you sure there isn't any test coverage? As far as I can tell, the tests from https://reviews.llvm.org/rL133503 are still in the tree.I looked at those, but none of them include the full pattern that decomposes into bswap and rol. I debugged through the X86 bswap.ll test and verified none of those cases make it through MatchBSwapHWord (they get handled in MatchBSwapHWordLow instead).>> I don't think there is any existing code to match this particular pattern in instcombine. I guess it wouldn't be hard to add, though.Yes, I guess the InstCombiner::MatchBSwap() function is very similar but not identical. It looks for patterns that match the bswap exactly, whereas the DAGCombiner version is looking for halfword swap patterns that decompose to a bswap and a 16-bit rotate. Jim On 12/8/16, 12:31 PM, "Friedman, Eli" <efriedma at codeaurora.org<mailto:efriedma at codeaurora.org>> wrote: On 12/8/2016 10:01 AM, Jim Lewis via llvm-dev wrote: I’m looking into problems with the function DAGCombiner::MatchBSwapHWord and had a couple questions on how to proceed (I’m new here, and to llvm). It looks like there was never any test coverage for this code, and it doesn’t match the patterns it claims to be looking for. I found a fix for this in our fork, but the fix is even worse in that it matches a whole bunch of invalid patterns as well as the canonical patterns. I have simple test cases that break both the existing code and our local fix. Are you sure there isn't any test coverage? As far as I can tell, the tests from https://reviews.llvm.org/rL133503 are still in the tree. I see equivalent matching code upstream in InstCombiner::MatchBSwap() (InstCombineAndOrXor.cpp), but not an easy way to re-use it AFAICT. Questions: --Is this functionality even worth fixing in DAGCombiner (ie, what is the probability that this complex pattern would re-emerge in codegen after the InstCombiner transform, weighed against the obviously error-prone nature of the matching code)? I don't think there is any existing code to match this particular pattern in instcombine. I guess it wouldn't be hard to add, though. -Eli -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- The information contained in this message is intended only for the recipient, and may be a confidential attorney-client communication or may otherwise be privileged and confidential and protected from disclosure. If the reader of this message is not the intended recipient, or an employee or agent responsible for delivering this message to the intended recipient, any dissemination or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify us by replying to the message and then deleting it from your computer. Please note that KnuEdge, Inc. reserves the right to monitor and review the content of any electronic message or information sent to or from KnuEdge employee e-mail addresses without informing the sender or recipient of the message. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161208/94355043/attachment.html>