Teach TargetInstrInfo::getInlineAsmLength to parse .space directives with integer arguments

[asb]

Summary

It's currently quite difficult to test passes like branch relaxation, which requires branches with large displacement to be generated. The .space assembler directive makes it easy to create arbitrarily large basic blocks, but getInlineAsmLength is not able to parse it and so the size of the block is not correctly estimated. Other backends (AArch64, AMDGPU) introduce options just for testing that artificially restrict the ranges of branch instructions (e.g. aarch64-tbz-offset-bits). Although parsing a single form of the .space directive feels inelegant, it does allow a more direct testing approach.

This patch adapts the .space parsing code from Mips16InstrInfo::getInlineAsmLength and removes it now the extra functionality is provided by the base implementation. CC @fhahn, @rengolin as this change might motivate dropping aarch64-{tbz,cbz,bcc}-offset-bits and updating the tests to use .space. I want to move this functionality to the generic getInlineAsmLength as 1) I need the same for RISC-V, and 2) I feel other backends will benefit from more direct testing of large branch displacements.

[sdardis]

lib/CodeGen/TargetInstrInfo.cpp

104–107 ↗

(On Diff #115007)

This code has unexpected behaviour in that it accepts a mixture of zero or more things that look like instructions or comments ending with an optional .space <int> directive. If the .space directive is parsed and there is not zero or more spaces followed by the null byte, the value of space is discarded even if it was a positive number.

This bug was present in the original version, but it didn't occur to my knowledge as ".space <num>" was the only form used as far as I can see.

I think it is better to parse this properly so that people don't get tripped up over when trying to use this.

Aside: we may also want to handle .align, .fill at some point.

[asb]

Update to address review comments from @sdardis (thanks, good catch).

The following will now produced a block with size 300+instlength:

.space 100 # comment
.space 200
addi r1, r2, 3

Unsupported parameters to space will still result in the directive being ignored, e.g. .space 1 << 20, .space 200+56. I'm wary of implementing any more .size variants, we don't want to reimplement the assembly parser and the motivating use case really is to make testing easier.

[asb]

Sorry for the noise, the previous version left out part of the patch.

[sdardis]

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.

lib/CodeGen/TargetInstrInfo.cpp

83–85 ↗

(On Diff #115060)

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.

94 ↗

(On Diff #115060)

Bug here: we don't advance the parser to the character past the '\n' or MAI.getSeparatorString() but we do change state...

101 ↗

(On Diff #115060)

...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.

111 ↗

(On Diff #115060)

We should also handle MAI.getSeparatorString() here but that relies on implementing this function as better parser.

llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp

 
 /// insertNoop - Insert a noop into the instruction stream at the specified
 /// point.
 void TargetInstrInfo::insertNoop(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MI) const {
   llvm_unreachable("Target didn't implement insertNoop!");
 }
 
+static bool isAsmComment(const char *Str, const MCAsmInfo &MAI) {
+  return strncmp(Str, MAI.getCommentString().data(),
+                 MAI.getCommentString().size()) == 0;
+}
+
 /// Measure the specified inline asm to determine an approximation of its
 /// length.
 /// Comments (which run till the next SeparatorString or newline) do not
 /// count as an instruction.
 /// Any other non-whitespace text is considered an instruction, with
 /// multiple instructions separated by SeparatorString or newlines.
 /// Variable-length instructions are not handled here; this function
 /// may be overloaded in the target code to do that.
+/// 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.
 unsigned TargetInstrInfo::getInlineAsmLength(const char *Str,
                                              const MCAsmInfo &MAI) const {
   // Count the number of instructions in the asm.
-  bool atInsnStart = true;
-  unsigned InstCount = 0;
+  bool AtInsnStart = true;
+  unsigned Length = 0;
   for (; *Str; ++Str) {
     if (*Str == '\n' || strncmp(Str, MAI.getSeparatorString(),
                                 strlen(MAI.getSeparatorString())) == 0) {
-      atInsnStart = true;
-    } else if (strncmp(Str, MAI.getCommentString().data(),
-                       MAI.getCommentString().size()) == 0) {
+      AtInsnStart = true;
+    } else if (isAsmComment(Str, MAI)) {
       // Stop counting as an instruction after a comment until the next
       // separator.
-      atInsnStart = false;
+      AtInsnStart = false;
     }
 
-    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();
+      if (strncmp(Str, ".space", 6) == 0) {
+        char *EStr;
+        int SpaceSize;
+        SpaceSize = strtol(Str + 6, &EStr, 10);
+        SpaceSize = SpaceSize < 0 ? 0 : SpaceSize;
+        while (*EStr != '\n' && std::isspace(static_cast<unsigned char>(*EStr)))
+          ++EStr;
+        if (*EStr == '\0' || *EStr == '\n' ||
+            isAsmComment(EStr, MAI)) // Successfully parsed .space argument
+          AddLength = SpaceSize;
+      }
+      Length += AddLength;
+      AtInsnStart = false;
     }
   }
 
-  return InstCount * MAI.getMaxInstLength();
+  return Length;
 }
 
 /// ReplaceTailWithBranchTo - Delete the instruction OldInst and everything
 /// after it, replacing it with an unconditional branch to NewDest.
 void
 TargetInstrInfo::ReplaceTailWithBranchTo(MachineBasicBlock::iterator Tail,
                                          MachineBasicBlock *NewDest) const {
   MachineBasicBlock *MBB = Tail->getParent();

llvm/trunk/lib/Target/Mips/Mips16InstrInfo.h

public:
   }
 
   // build the proper one based on the Imm field
 
   const MCInstrDesc& AddiuSpImm(int64_t Imm) const;
 
   void BuildAddiuSpImm
     (MachineBasicBlock &MBB, MachineBasicBlock::iterator I, int64_t Imm) const;
-
-  unsigned getInlineAsmLength(const char *Str,
-                              const MCAsmInfo &MAI) const override;
 private:
   unsigned getAnalyzableBrOpc(unsigned Opc) const override;
 
   void ExpandRetRA16(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
                    unsigned Opc) const;
 
   // Adjust SP by Amount bytes where bytes can be up to 32bit number.
   void adjustStackPtrBig(unsigned SP, int64_t Amount, MachineBasicBlock &MBB,

llvm/trunk/lib/Target/Mips/Mips16InstrInfo.cpp

case Mips::LwRxSpImmX16:
     return isInt<16>(Amount);
   case Mips::AddiuRxRyOffMemX16:
     if ((Reg == Mips::PC) || (Reg == Mips::SP))
       return isInt<16>(Amount);
     return isInt<15>(Amount);
   }
   llvm_unreachable("unexpected Opcode in validImmediate");
 }
-
-/// Measure the specified inline asm to determine an approximation of its
-/// length.
-/// Comments (which run till the next SeparatorString or newline) do not
-/// count as an instruction.
-/// Any other non-whitespace text is considered an instruction, with
-/// multiple instructions separated by SeparatorString or newlines.
-/// Variable-length instructions are not handled here; this function
-/// may be overloaded in the target code to do that.
-/// 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.
-unsigned Mips16InstrInfo::getInlineAsmLength(const char *Str,
-                                             const MCAsmInfo &MAI) const {
-
-  // Count the number of instructions in the asm.
-  bool atInsnStart = true;
-  unsigned Length = 0;
-  for (; *Str; ++Str) {
-    if (*Str == '\n' || strncmp(Str, MAI.getSeparatorString(),
-                                strlen(MAI.getSeparatorString())) == 0)
-      atInsnStart = true;
-    if (atInsnStart && !std::isspace(static_cast<unsigned char>(*Str))) {
-      if (strncmp(Str, ".space", 6)==0) {
-        char *EStr; int Sz;
-        Sz = strtol(Str+6, &EStr, 10);
-        while (isspace(*EStr)) ++EStr;
-        if (*EStr=='\0') {
-          DEBUG(dbgs() << "parsed .space " << Sz << '\n');
-          return Sz;
-        }
-      }
-      Length += MAI.getMaxInstLength();
-      atInsnStart = false;
-    }
-    if (atInsnStart && strncmp(Str, MAI.getCommentString().data(),
-                               MAI.getCommentString().size()) == 0)
-      atInsnStart = false;
-  }
-
-  return Length;
-}