[PATCH] D132222: [Assignment Tracking][3/*] Add DIAssignID metadata boilerplate

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:4419-4420
+bool LLParser::parseDIAssignID(MDNode *&Result, bool IsDistinct) {
+  if (!IsDistinct)
+    return Lex.Error("missing 'distinct', required for !DIAssignID()");
+
----------------
Orlando wrote:
> Orlando wrote:
> > @jmorse I was writing up the verifier tests you asked for and noticed that the verifier check that DIAssignID nodes are distinct doesn't fire because LLParser (here) gets there first. Do you have a a preference as to whether the check stays here or is removed (relying on the verifier to catch the error if it came from IR in a file)?
> > 
> > On the one hand, it's not a syntactic requirement for DIAssignID to be distinct. OTOH, it's a semantic requirement and we can catch it here.
> > 
> > Either way, we should keep the verifier check given a pass may possibly somehow generate non-distinct DIAssignID.
> It's also not possible to cause verifier failures by adding operands to DIAssignID (see code below), so I've just added a test the parser parses things expectedly in a new directory parse-and-verify.
Blurgh. I think ultimately we'll just create more problems for ourselves if we make it so that you can't express IR that can be represented in memory -- AFAIUI that's why you can parse broken IR, and then it's the Verifier that complains about things like "use before def".

There's probably more value in ensuring the Verifier path is covered by a test, to ensure we catch the (rare but possible) scenarios of non-distinct DIAssignIDs. We don't gain anything by doing it the other way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132222/new/

https://reviews.llvm.org/D132222