[PATCH] Fix for PR20104. Enable GAS compliant coprocessor register name.

Renato Golin renato.golin at linaro.org
Tue Jun 24 03:25:41 PDT 2014


Hi Andrey,

Addressing both mine and Stepan's comments, the patch looks good to me.

cheers,
--renato

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3093
@@ +3092,3 @@
+
+  if (Name.startswith("cr")) // For gas compatibility
+    Name = Name.drop_front();
----------------
Stepan Dyatkovskiy wrote:
> Please place comment above "if" statement:
> // For gas compatibility
> if ...
Since we use CoprocOp for the first char, I'd do in a similar way for the GNU compatibility.

Alternatively, assuming the name will always come lower case, and since you already tested for the first char, I'd just test:

  if (Name[1] == 'r') {
    Name = Name.drop_front();
    Name[0] = CoprocOp;
  }

Why the second line? Because from this point onwards, the string will be 'rN' rather than 'cN', which is why you moved the CoprocOp test upwards, but that is not future proof, and anyone writing checks below that line will get confused, or produce confusing code, such as:

  if (Name[0] == 'r') ...

which doesn't help future developers.

http://reviews.llvm.org/D4254






More information about the llvm-commits mailing list