[PATCH] D33128: [AsmParser] Mnemonic Spell Corrector

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Sjoerd,

I think this is a nice addition, and if we can trim all the duplication/public issues, the extra computation makes no difference, since this is going to run only when the compiler is on its way out.

The things I'd change from this, you probably already know, but just for completeness sake: :)

- Add the check functions to a cpp file, probably an already existing one that handles asm errors
- Keep things private, and if needed, maybe even duplicate the info (computation is not a problem here)
- Show what change you'd need to do to the original table (maybe a new column?), so we can see what would be the cost
- As you said, some tests would be nice

The only thing I worry here is that instructions rarely behave like text. So the Levenshtein distance may work for "addx -> add" but I'm expecting a lot of bad proposals, maybe enough to make the whole exercise worse than not having it.

This could be really nice if expanded to understand the operands, for example, and restrict to a subset (say, only opcodes that can use quad-regs), or something. In that case, the number of false positives would be lower and the quality of the proposals much better. But obviously, this wouldn't fit this patch.

So, if we have a plan to really make this target agnostic, but maybe with some target-hooks for the special filters, then I think it's a good idea. If the plan is to just use the Levenshtein distance, then it may be too much hassle.

Makes sense?

cheers,
--renato


https://reviews.llvm.org/D33128