This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix builtins-x86.c test where it wasn't using immediates. NFC
ClosedPublic

Authored by russell.gallop on Jun 4 2019, 3:48 AM.

Details

Summary

Some of these test cases were using a variable instead of a literal so were not generating the immediate form of the corresponding instruction.

Diff Detail

Event Timeline

russell.gallop created this revision.Jun 4 2019, 3:48 AM

.. or?
Is this fixing a current test failure?

.. or?

I'm not sure what you mean.

Is this fixing a current test failure?

No. The test is not failing, but it is not doing what was intended as these builtins are for generating the immediate form of the corresponding instruction and they were generating actually generating a register form.

This test doesn't check the instructions generated are correct, but fixing that is a bigger task...

Is the compiler missing a check that these parameters are immediates?

Is the compiler missing a check that these parameters are immediates?

I don't think that there can be a check, or this would have been noticed.

I don't know whether this is possible and/or desirable. Do users use one version of the builtin and want the compiler to decide whether to use the immediate form?

.. or?

I'm not sure what you mean.

Is this fixing a current test failure?

No. The test is not failing, but it is not doing what was intended as these builtins are for generating the immediate form of the corresponding instruction and they were generating actually generating a register form.

Then the test should be failing? Or is the current form also legal?

This test doesn't check the instructions generated are correct, but fixing that is a bigger task...

Is the compiler missing a check that these parameters are immediates?

I don't think that there can be a check, or this would have been noticed.

I don't know whether this is possible and/or desirable. Do users use one version of the builtin and want the compiler to decide whether to use the immediate form?

See https://llvm.org/docs/LangRef.html#parameter-attributes immarg

Then the test should be failing? Or is the current form also legal?

Hmm, builtin_ia32_insertps128() errors if you pass a variable, but these don't (e.g.):
mytest.c:122:13: error: argument to '
builtin_ia32_insertps128' must be a constant integer

tmp_V4f = __builtin_ia32_insertps128(tmp_V4f, tmp_V4f, tmp_i);

I'll have a look and see if there is a reason why these don't fail in the same way (which would make the test fail in it's current form).

We support non immediate on these because gcc does.

I'll have a look and see if there is a reason why these don't fail in the same way (which would make the test fail in it's current form).

These do not have the "I" prefix (// I -> Required to constant fold to an integer constant expression.) in BuiltinsX86.def which is probably why they don't error.

Checking against the LLVM side, there is a comment in IntrinsicsX86.td:

// Oddly these don't require an immediate due to a gcc compatibility issue.
def int_x86_mmx_pslli_w : GCCBuiltin<"__builtin_ia32_psllwi">,
            Intrinsic<[llvm_x86mmx_ty], [llvm_x86mmx_ty,
                       llvm_i32_ty], [IntrNoMem]>;

That comment was added recently by Craig in r355993. So it looks like they should work with either for compatibility.

We support non immediate on these because gcc does.

Thanks. Your comment crossed in mid-air. Okay, so is this test worth changing, or should it have both versions (immediate and non-immediate)?

We support non immediate on these because gcc does.

Thanks. Your comment crossed in mid-air. Okay, so is this test worth changing, or should it have both versions (immediate and non-immediate)?

Both, i guess, with a comment as to why they are the way they are.

Re-added test cases using variables and added comment. This now tests both formats.

This revision is now accepted and ready to land.Jun 6 2019, 10:32 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2019, 2:48 AM