This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Improve diagnostics related to target attributes
ClosedPublic

Authored by GBuella on May 7 2018, 11:58 AM.

Details

Summary

When requirement imposed by target attributes on functions
are not satisfied, prefer printing those requirements, which
are explicitly mentioned in the attributes.

This makes such messages more useful, e.g. printing avx512f instead of avx2
in the following scenario:

$ cat foo.c
static inline void __attribute__((__always_inline__, __target__("avx512f")))
x(void)
{
}

int main(void)
{
	        x();
}
$ clang foo.c
foo.c:7:2: error: always_inline function 'x' requires target feature 'avx2', but would be inlined into function 'main' that is compiled without support for 'avx2'
        x();
	^
1 error generated.

bugzilla: https://bugs.llvm.org/show_bug.cgi?id=37338

Diff Detail

Repository
rL LLVM

Event Timeline

GBuella created this revision.May 7 2018, 11:58 AM
craig.topper added inline comments.May 7 2018, 3:05 PM
lib/CodeGen/CodeGenFunction.cpp
2271 ↗(On Diff #145511)

'&' should be in front of the variable name not after the type.

2305 ↗(On Diff #145511)

Please run clang-format on this. There's some weird indentation happening.

2345 ↗(On Diff #145511)

Can we call getAttr here and pass the result of that instead of the TargetDecl? We know the attribute exists. Then rename getFunctionTargetAttrs and make it only responsible for removing unknown features. Push its other call site back into the 'if' there. This removes the need to return an empty ParsedTargetAttr.

lib/CodeGen/CodeGenModule.cpp
4998 ↗(On Diff #145511)

Open curly brace should be on previous line.

5024 ↗(On Diff #145511)

!ParsedAttr.Features.empty()

GBuella updated this revision to Diff 145680.May 8 2018, 6:22 AM
craig.topper added inline comments.May 8 2018, 8:45 AM
lib/CodeGen/CodeGenFunction.cpp
2346 ↗(On Diff #145680)

This and the next line are indented funny.

craig.topper added inline comments.May 8 2018, 10:46 AM
lib/CodeGen/CodeGenFunction.cpp
2342 ↗(On Diff #145680)

Rather than walking the ParsedAttr.Features for each feature in the map. And having to shift the ReqFeatures vectors sometimes. How about doing this

-Walk through all features in ParsedAttr, for each feature with a +, query the callee feature map. If it's enabled there, push it to ReqFeatures.
-Walk through all features in the callee feature map and if enabled push those.

This will lead to duplicates in the list, but all the explicitly mentioned features will be listed first.

GBuella updated this revision to Diff 145876.May 9 2018, 12:59 AM

This looks pretty good to me. @echristo what do you think?

I think this will work, one inline comment. Might also be good to get a few different test cases, e.g. one where we're not seeing the alphabetically first as the minimum :)

lib/CodeGen/CodeGenModule.h
1085 ↗(On Diff #145876)

Needs a block comment saying what it does :)

GBuella updated this revision to Diff 147775.May 21 2018, 5:53 AM

Fixed a horrible bug in the patch. Adding a ref to temporary string is not a wise thing to do, so I had to remove this line:

ReqFeatures.push_back(F.substr(1));

Now the ReqFeatures vector can also refer to strings starting with '+'

Also, added a few tests and comments.

GBuella marked an inline comment as done.May 21 2018, 5:54 AM
GBuella added a subscriber: ashlykov.

I think you can pass StringRef(F).substr(1). That won't create a temporary string. It will just create a StringRef pointing into the middle of an existing std::string stored in the parsed attributes.

GBuella updated this revision to Diff 147984.May 22 2018, 4:56 AM

I think you can pass StringRef(F).substr(1). That won't create a temporary string. It will just create a StringRef pointing into the middle of an existing std::string stored in the parsed attributes.

Yes, that seems to work.

craig.topper added inline comments.Jun 5 2018, 8:26 AM
test/CodeGen/target-features-error-2.c
39 ↗(On Diff #147984)

The 4 compare functions here are basically the same for the purpose of this check. What value do we get by testing all 4 of them?

GBuella added inline comments.Jun 5 2018, 8:29 AM
test/CodeGen/target-features-error-2.c
39 ↗(On Diff #147984)

Which four more specifically?

craig.topper added inline comments.Jun 5 2018, 9:01 AM
test/CodeGen/target-features-error-2.c
39 ↗(On Diff #147984)

_mm_cmp_ps, _mm_cmp_ss, _mm_cmp_pd, _mm_cmp_sd. Or the cases for NEED_AVX_2-5. They're all basically checking the same thing. There is nothing different about the command lines other than the -D NEED_AVX

GBuella updated this revision to Diff 150018.Jun 5 2018, 11:49 AM
GBuella marked an inline comment as done.
GBuella added inline comments.
test/CodeGen/target-features-error-2.c
39 ↗(On Diff #147984)

Yes, I guess we could just remove NEED_AVX_3,4,5

This revision is now accepted and ready to land.Jun 5 2018, 1:58 PM
This revision was automatically updated to reflect the committed changes.