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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

LGTM, if you're OK with the current configuration of where verifier checks / tests covering them happens.



================
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:
> 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.
Sooo, just because we can't create a non-distinct one now, doesn't mean someone won't invent a way of doing it in the future that we don't notice -- the benefit of having tests is that they're anchored to behaviour, rather than a particular implementation. (And in this case, we test that the verifier path actually works).

Leaving the operand check in the parser seems fine to me, there's much less opportunity for things to be subtly broken if DIAssignIDs accidentally carried operands.


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

https://reviews.llvm.org/D132222