[PATCH] D21723: [RFC] Enhance synchscope representation

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1784
+  }
+}
+
----------------
t-tye wrote:
> If the sync scope block is present, should be required to have at least one entry? That would be consistent with the assert for checking for duplicate blocks above (since it relies on SSIDs not being empty), and the writer seems to ensure that it never generates empty blocks. (In practice there will always be 2 entries for the singlethread and empty system scopes, but that could change in the future.)
> 
> ```
> if (SSIDs.empty())
>   return error("Invalid empty sync scope names block");
> ```
> 
> 
I did not see that SSIDs.empty() being reported as an error after the loop has processed the block's entries..


================
Comment at: lib/CodeGen/MIRParser/MIParser.cpp:2447-2459
+bool MIParser::parseString(std::string &Result) {
+  if (expectAndConsume(MIToken::dquote))
+    return true;
+  if (!Token.is(MIToken::Identifier))
+    return error("expected identifier");
+
+  Result = Token.stringValue();
----------------
This does not match the syntax of a string in the LLVM IR parser (see LLLexer::ReadString()). It parses up to the next double quote, and then unescapes // and /nn.

Seems MIR should support the same values as the LLVM IR otherwise the MIR will give errors parsing sync scope names that are legal in the LLVM IR.

Similarly the printing of MIR syncscope should match any escaping () done.

Alternatively LLVM IR should enforce that the sync scope name strings are just identifiers in double quotes, and the documentation for sync scope updated to state what sync scope name strings are allowed.


https://reviews.llvm.org/D21723