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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando 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()");
+
----------------
jmorse wrote:
> 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.
Deleting the is-distinct check from the parser sounds reasonable to me. Hmm, actually, in D132222 I only added `DIAssignID::getDistinct` and not `DIAssignID::get`. So I'm not actually sure that it _is_ possible to create a non-distinct `DIAssignID`.

Judging by other metadata parsing functions I think it should be okay to leave the zero-operands check in the parser. Are you happy with that? In addition, similarly to the `distinct` issue above, I don't think there's actually an API available to create a DIAssignID with operands. Maybe it's possible if someone tried really hard, but I'm not sure how.


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

https://reviews.llvm.org/D132222