Page MenuHomePhabricator

Hexagon: Add ImmArg to intrinsics
ClosedPublic

Authored by arsenm on Feb 18 2019, 8:05 AM.

Details

Reviewers
kparzysz
Summary

I found these by asserting in clang for any GCCBuiltin that doesn't
require mangling and requires a constant for the builtin. This means
that intrinsics are missing which don't use GCCBuiltin, don't have
builtins defined in clang, or were missing the constant annotation in
the builtin definition.

Diff Detail

Event Timeline

arsenm created this revision.Feb 18 2019, 8:05 AM

I don't really understand the summary, could you elaborate on how the asserting implies the conclusion (the part that follows "This means that..."). Also, what does that part mean on its own? "Intrinsics are missing that don't use GCCBuiltin...", meaning that you'd expect some intrinsics that don't use GCCBuiltin, but they're not defined?

I don't really understand the summary, could you elaborate on how the asserting implies the conclusion (the part that follows "This means that..."). Also, what does that part mean on its own? "Intrinsics are missing that don't use GCCBuiltin...", meaning that you'd expect some intrinsics that don't use GCCBuiltin, but they're not defined?

The assert can't catch all possible cases which may exist, so this may not be complete without someone with knowledge of these intrinsics auditing them.

GCCBuiltin is an annotation on the LLVM IR intrinsic which doesn't really do anything on its own. GCCBuiltin also doesn't support intrinsics that use name mangling (you would need to be able to specify one for each mangling you want to support in the frontend). 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.

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).

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'm not expecting this patch to be complete. Most of the intrinsic definitions are actually auto-generated, so we'll need to update the generating scripts to include these new annotations. What concerns me is that you claim that some things are missing, which could indicate that there are some issues with the generators. I'm trying to understand exactly what the missing pieces are, the contents of this patch are actually of secondary importance.

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.

I'm not expecting this patch to be complete. Most of the intrinsic definitions are actually auto-generated, so we'll need to update the generating scripts to include these new annotations. What concerns me is that you claim that some things are missing, which could indicate that there are some issues with the generators. I'm trying to understand exactly what the missing pieces are, the contents of this patch are actually of secondary importance.

I'm not claiming I know it's incomplete, it's just there are reasons the method for finding them can't catch 100% of cases. In hexagon's case in particular GCCBuiltin seems to be consistently used, so the coverage is probably better

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.

S2_insert is covered, it's just handled inside Hexagon_i32_i32i32i32i32_Intrinsic

kparzysz accepted this revision.Feb 18 2019, 12:29 PM

I see. Thanks for looking into it.

In any case, most of this file will be regenerated with the necessary annotations anyways, so if something's missing now it will be included later on.

This revision is now accepted and ready to land.Feb 18 2019, 12:29 PM
arsenm closed this revision.Mar 13 2019, 12:46 PM

r356092