[PATCH] D58356: Hexagon: Add ImmArg to intrinsics

Mon Feb 18 09:55:16 PST 2019

arsenm added a comment.

In D58356#1401354 <https://reviews.llvm.org/D58356#1401354>, @kparzysz wrote:

> In D58356#1401321 <https://reviews.llvm.org/D58356#1401321>, @arsenm wrote:
>
> > There has to be a corresponding builtin definition explicitly defined in clang, which also needs to correctly mark the required constant operands. The test coverage for which operands need to be constants is spotty or nonexistent for these, so those are possibly missing in the builtin definition.
>
>
> So you added the assertion to clang and ran clang-check?  This patch is missing information for S2_insert (that's one case that found by just looking at it), but the builtin for S2_insert is both marked properly in BuiltinsHexagon.def, and is run through clang codegen tests.
>
> About this thing in particular:
>
> > The test coverage for which operands need to be constants is spotty or nonexistent for these, so those are possibly missing in the builtin definition.
>
> Yes, we only test a few cases to see if the SemaCheck machinery works, I'm not sure if that's what you're referring to.  There are a lot more builtins tested for codegen, which should still go through the same checks (this includes the S2_insert builtin, which is not annotated in this patch).

Yes, there isn't test coverage to make sure each builtin is properly marked for constant arguments.

> Was the S2_insert omitted because you missed it when you were editing the .td file, or because your testing didn't find it?  From your description so far, all I really know is that you added an assertion somewhere, the rest is really vague.

I didn't find it, I'll try to figure out why.

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

https://reviews.llvm.org/D58356