This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Update min-legal-vector width based on function argument and return types
ClosedPublic

Authored by craig.topper on Sep 24 2018, 3:32 PM.

Details

Summary

This is a continuation of my patches to inform the X86 backend about what the largest IR types are in the function so that we can restrict the backend type legalizer to prevent 512-bit vectors on SKX when -mprefer-vector-width=256 is specified if no explicit 512 bit vectors were specified by the user.

This patch updates the vector width based on the argument and return types of the current function and from the types of any functions it calls. This is intended to make sure the backend type legalizer doesn't disturb any types that are required for ABI.

Diff Detail

Repository
rC Clang

Event Timeline

craig.topper created this revision.Sep 24 2018, 3:32 PM
rnk added a comment.Oct 1 2018, 11:27 AM

Code looks fine, but attribute testing is always a pain.

test/CodeGen/aarch64-neon-3v.c
14

These attribute changes don't appear to test anything. They don't say anything about the min-legal-vector-width. It's unfortunate that LLVM attribute syntax is so filecheck unfriendly, but for now, I think you need to check for #0, #1, etc attribute definitions at the end of each .c file.

test/CodeGen/x86-vector-width.c
52

I'd look for define {{.*}}@foo{{.*}} #0 to be a bit more precise.

test/CodeGenOpenCL/fpmath.cl
50–51

Does this actually work? Shouldn't these be NODIVOPT-SAME: "correctly-rounded-divide-sqrt-fp-math"="false"?

Couple of comments/questions:

a) How do you want these attributes to be handled in merging/inlining? Also, are they a failure on module linking? In general, how do they work with LTO?
b) Could use some more comments when we're adding/merging the attributes in IR generation as well.

Address Reid's comments. Add a comment with a list of all things that currently effect the vector width attribute emitted in IR.

For inlining, we update the caller's attribute during merging to ensure it is at least as large as the callee that is being inlined. This is required for always_inline of the intrinsics. We probably want a way to limit inlining, but that would effect the inlining decision. If the decision has been made to inline we have to take the max.

For LTO I don't have an answer. What do we do for things like target features and cpu today?

Add back the new test file that I lost in the previous update

rnk added a comment.Oct 22 2018, 3:25 PM

Address Reid's comments. Add a comment with a list of all things that currently effect the vector width attribute emitted in IR.

For inlining, we update the caller's attribute during merging to ensure it is at least as large as the callee that is being inlined. This is required for always_inline of the intrinsics. We probably want a way to limit inlining, but that would effect the inlining decision. If the decision has been made to inline we have to take the max.

For LTO I don't have an answer. What do we do for things like target features and cpu today?

I think your comments about the behavior w.r.t. inlining are enough to describe what happens during LTO. I don't want to speak for Eric, but I think you've answered his questions.

echristo accepted this revision.Oct 23 2018, 5:20 PM
In D52441#1271545, @rnk wrote:

Address Reid's comments. Add a comment with a list of all things that currently effect the vector width attribute emitted in IR.

For inlining, we update the caller's attribute during merging to ensure it is at least as large as the callee that is being inlined. This is required for always_inline of the intrinsics. We probably want a way to limit inlining, but that would effect the inlining decision. If the decision has been made to inline we have to take the max.

For LTO I don't have an answer. What do we do for things like target features and cpu today?

I think your comments about the behavior w.r.t. inlining are enough to describe what happens during LTO. I don't want to speak for Eric, but I think you've answered his questions.

Yes, it'll be the same. As a note, the inlining widening must happen after the check for subtarget features.

Otherwise we talked about this at the conference and LGTM.

This revision is now accepted and ready to land.Oct 23 2018, 5:20 PM
This revision was automatically updated to reflect the committed changes.