[llvm] [TableGen] fix tlbgen for EncodingByHwMode (PR #84906)

Jason Eckhardt via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 18 09:38:36 PDT 2024


nvjle wrote:

 
Hi @superZWT123 ,
> What worries me the most is what you mentioned about seeing **miscompiles** in your backend. If you can provide some error information, I believe I could better pinpoint where the problem lies. Alternatively, after I split the patch into two parts, it might be easier to reproduce and locate the problem on your backend, but this would probably require your continued assistance in reviewing the split patches. In fact, I enabled the current patch in my backend and assigned the 'EncodingByHwMode' attribute to seven or eight instructions to resolve encoding conflicts. So far, I have not encountered any problems. 

  As mentioned earlier, I have not yet had time to nail down precisely what is happening here, and whether related to the encoder changes per-se or something on our end.  It has taken all of my time so far just integrating and reviewing all of the changes-- another reason independent/small changes are important. I should have this nailed down soon.

> 
> Also, I wanted to ask if I should split this pull request into two new pull requests, or just split the commit of this pull request into two? Since this branch has already fallen significantly behind the main branch, I might rebase it and then resubmit two new pull requests. I’m not sure if this is the correct approach.

  Submitting two new pull requests (after rebasing) is what I had in mind. Two logically independent patches which can be reviewed separately and on different timelines.

> 
> If there are any points that I haven't replied to, please point them out. Thank you very much!

  Regarding the `DecoderEmitter` part of the patch: One crucial point not touched on above (but this should be discussed in the follow-up decoder-related PR)-- is that decoder suppression _already_ exists in TOT, and this part of the patch seems entirely unaware of that. Indeed it essentially just cancels/undoes some of the existing support (as if the merge conflict was just resolved in favor of downstream without consideration of the existing functionality). 
This isn't just the command line option, but the actual suppression code. See the `bar` example in `HwModeEncodeDecode2.td` pointed to earlier, where the current patch regresses. That unit test was deliberately constructed to make sure suppression occurs-- but this patch removed that as well.  As mentioned above, it appears that the decoder patch is trying to achieve what TOT already does (and TOT does more thoroughly and cleanly)-- just with a different (blank) name for the `AllModes` tables. This is what I meant above when I said "_The current TOT code seems to entirely obviate the decoder part of the patch_". The follow-up decoder patch (if needed at all) should address those points: explain what is meant to be achieved relative to TOT; how/why TOT does not already achieve that aim; ensure no suppression regressions.




https://github.com/llvm/llvm-project/pull/84906


More information about the llvm-commits mailing list