Page MenuHomePhabricator

[RFC][mips][llvm-exegesis] Fix missing TargetStreamer in the Streamer for reading snippets
AbandonedPublic

Authored by mstojanovic on Dec 6 2019, 11:33 AM.

Details

Summary

To extract instructions from custom code snippets, llvm-exegesis creates a minimal parser in readSnippets(). The parser doesn't initialize the TargetStreamer because it's not used on X86 (and most other architectures) but the MipsAsmParser assumes that it is initialized which creates a problem when extending the implementation.

One solution could be to add the initialization of the TargetStreamer, but the functions that can be used for that usually create arch specific streamers like MipsTargetELFStreamer which (again, unlike most other architectures) needs an Assembler which isn't initialized. Since the BenchmarkStreamer only inherits the MCStreamer it's unclear what type of reorganization would be needed to accommodate additional information like the Assembler.

Another solution, submitted here, is to check whether TargetStreamer returns a null reference (which AFAIK isn't allowed by definition but still happens) from the Mips side and skip accessing it. This might miss edge cases or future uses where the TargetStreamer is erroneously missing but it works for this case. The TargetStreamer is usally used in cases where some additional instructions need to be emitted, but since the tool is only reading the snippets most of the code should be skipped, so there shouldn't be problems with not initilizing variables like TOut. This is just a basic patch which enables running exegesis with an empty file: llvm-exegesis -mode latency -snippets-file=/dev/null or a file containing simple instructions. To be able to run this for DIV for example, TOut in expandDivRem() would have to adapted in a similar way, and so on for other instructions. A possible improvement of this solution could be to use dyn_cast to determine the origin of the Streamer instead of assuming that it's from exegesis if the TargetStreamer isn't initialized. This would requires dyn_cast to be implemented for streamers which it currently isn't.

These are some ideas that don't constitute a perfect solution so I hope you can help me get a clearer insight into how to a better one might look like.

Diff Detail

Event Timeline

mstojanovic created this revision.Dec 6 2019, 11:33 AM

AsmParsers in other architectures have references and use a TargetStreamer too. What's a difference in MIPS? Does MIPS need a TargetStreamer in places where other architectures do not use it?

Yes, other targets do have it but use it quite sparingly. X86 almost never uses it but there are some edge cases when parsing certain directives. For example, this line will trigger an assert fail because of an uninitialized TargetStreamer on X86:

echo ".cv_fpo_proc foo 4" | llvm-exegesis -mode latency -snippets-file=-

MIPS on the other hand uses TargetStreamer right at the start of the MipsAsmParser constructor and throughout the code used for parsing. The design of this MIPS code is quite different from other architectures (always relying on TargetStreamer or Assembler). I'm not sure if this was out of necessity or if it just evolved in to different path. Either way, the current code doesn't facilitate simple instruction extraction without all the necessary elements being in place.

@gchatelet @courbet How do you think the example I mentioned in the previous comment should be handeled on X86? This might inform us what (if anything) should be changed for MIPS.

@gchatelet @courbet How do you think the example I mentioned in the previous comment should be handeled on X86? This might inform us what (if anything) should be changed for MIPS.

Thx for taking the time to explain the offered solution in great details.
I need to see how we can adapt the code to get the Streamer created in the first place because it looks like it should be set up properly.
I've been busy the last few days but I'll have a look shortly (same for the other patch you sent my way).

My apologies for the delay.

I've created D71468 which should fix the issue for all backend at once and prevent hacking in MipsAsmParser.cpp

mstojanovic abandoned this revision.Dec 13 2019, 9:35 AM