[PATCH] D89184: Support complex target features combinations

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a subscriber: echristo.
tra added a comment.
This revision is now accepted and ready to land.

@echristo : FYI, just in case you have an opinion on bringing required feature constraint checks one step closer to being Turing-complete. :-)

LGTM as far as syntax and use for NVPTX goes. No opinion on whether it **should** go into codegen.
While the change appears to be sensible on general principles, I'm not aware of any specific case where it's actually needed.
It would help if you could elaborate on why you need this functionality.



================
Comment at: clang/unittests/CodeGen/CheckTargetFeaturesTest.cpp:16
+  };
+  ASSERT_TRUE(doCheck("A|B,C|D", "A"));
+  ASSERT_FALSE(doCheck("(A|B),(C|D)", "A"));
----------------
LiuChen3 wrote:
> pengfei wrote:
> > tra wrote:
> > > Is `doCheck("A,B,C,D", ...)`  expected to work? I'd add test cases for that, too.
> > > 
> > > 
> > I may not get your point here.
> > We added the unittest to check the correctness for the function. It's an easier and more flexible way checking for complex functions than using llc.
> > I don't know if there's criteria for the unittests.
> I think @tra 's mean is adding tests like ASSERT_FALSE(doCheck("A,B,C,D", "A")); .
Yes. I should've phrased it better.  I meant tests w/o parens in the feature requirements.


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

https://reviews.llvm.org/D89184