[llvm] [TableGen] Use bitwise operations to access HwMode ID. (PR #88377)

Jason Eckhardt via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 17 14:59:48 PDT 2024


nvjle wrote:

Though I have only briefly reviewed this (beyond what I did in the original series a few weeks ago), we did apply the patches locally to a production downstream back end. Unfortunately, we're getting a number of failures in both disassembler and encoding tests. 

Just a quick diff of the `MyTargetMCCodeEmitter.inc` shows ~15000 differences compared to pre-patch. It is impossible to tell immediately what is incorrect on such a complicated target/ISA/multiple-encodings, and we're busy on other high priority work at the moment. I/we will attempt to isolate this in the next few days, but it may be that we are unable with current resources in a short amount of time. If this is committed as is, our target (and possibly others downstream) will be broken on the next upstream rebase. Maybe it is something easy, we will try our best to figure it out.

One immediate hunch is that our back end has long instruction words of a few different widths-- starting at 128 bits and only getting larger from there. This means that the decoder and encoder emitters switch to APInt mode, which may or may not have been carefully tested in your patch. No in-tree back ends have long words, but at least a few downstream ones do-- and I'm not sure if yours does. 

As an aside, it is difficult to avoid breakage in different downstream back ends for features that are not (yet) used in any in-tree back ends. Ours will be upstreamed eventually, but the timeline is unknown. Until then, it is possible changes like this in one or another downstream target will need to stay local (we have many).

Also, I agree with Craig that the pipe-cleaning/test changes in the RISCV back end need to be removed. They also lengthen and obscure the core patch. The unit tests should cover all of that functionality. If possible, please also make sure the long-word APInt modes of the encoder (and anywhere else changes were made) are covered in the testing. It isn't possible, of course, to do actual disassembler/encoder tests since there is no in-tree back end, but at least you can sanity check the encoder emitter output for that mode (I didn't see it in the current tests).


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


More information about the llvm-commits mailing list