[PATCH] D37798: Teach TargetInstrInfo::getInlineAsmLength to parse .space directives with integer arguments

[Simon Dardis via Phabricator via llvm-commits]

sdardis accepted this revision.
sdardis added a comment.
This revision is now accepted and ready to land.

This LGTM but the original parser suffers from a bug / efficiency issue where we don't advance Str but do change state. This causes things like ".space 1000;nop;.space 1000"--when we permit the target specific separator to come after a .space <num> directive--to be measured as 1008 bytes rather than 2004.

Given this particular piece of code has existed for 10 years in this state, I don't think it's really an issue for this patch.



================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:83-85
+/// We implement the special case of the .space directive taking only an
+/// integer argument, which is the size in bytes. This is used for creating
+/// inline code spacing for testing purposes using inline assembly.
----------------
We implement a special case of the .space directive which takes only a single integer argument in base 10 that is the size in bytes. This is a restricted form of the GAS directive in that we only interpret simple--i.e. not a logical or arithmetic expression--size values without the optional fill value. This is primarily used for creating arbitrary sized inline asm blocks for testing purposes.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:94
                                 strlen(MAI.getSeparatorString())) == 0) {
-      atInsnStart = true;
-    } else if (strncmp(Str, MAI.getCommentString().data(),
-                       MAI.getCommentString().size()) == 0) {
+      AtInsnStart = true;
+    } else if (isAsmComment(Str, MAI)) {
----------------
Bug here: we don't advance the parser to the character past the '\n' or MAI.getSeparatorString() but we do change state...


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:101
 
-    if (atInsnStart && !std::isspace(static_cast<unsigned char>(*Str))) {
-      ++InstCount;
-      atInsnStart = false;
+    if (AtInsnStart && !std::isspace(static_cast<unsigned char>(*Str))) {
+      unsigned AddLength = MAI.getMaxInstLength();
----------------
...so here we're looking at the '\n' or start of MAI.getSeparatorString() but our state says we're at the start of an instruction. I think this should be addressed in a separate patch.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:111
+        if (*EStr == '\0' || *EStr == '\n' ||
+            isAsmComment(EStr, MAI)) // Successfully parsed .space argument
+          AddLength = SpaceSize;
----------------
We should also handle  MAI.getSeparatorString() here but that relies on implementing this function as better parser.


https://reviews.llvm.org/D37798