Page MenuHomePhabricator

AMDGPU: Stop setting attributes based on TargetOptions
ClosedPublic

Authored by arsenm on Dec 10 2019, 4:49 AM.

Details

Reviewers
rampitec
Summary

Having arbitrary passes looking at the TargetOptions is pretty
messy. This was also disregarding if a function already had an
explicit attribute setting on it. opt/llc now add the attributes to
functions that don't specify the attribute. clang and lld do not call
the function to do this, which they maybe should.

This was also treating unsafe-fp-math as implying the others, and
setting the other attributes based on it. This is not done anywhere
else, and I'm not sure is correct based on the current description of
the option bit.

Effectively reverts 1d8cf2be89087a2babc1dc38b16040fad0a555e2

Diff Detail

Event Timeline

arsenm created this revision.Dec 10 2019, 4:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 4:49 AM

clang and lld do not call the function to do this, which they maybe should.

Should we first fix clang then?

clang and lld do not call the function to do this, which they maybe should.

Should we first fix clang then?

They probably shouldn't be using these flags, but I don't know where to diagnose it either. They can properly set the function attributes themselves. The purpose of these flags is to help writing lit tests

clang and lld do not call the function to do this, which they maybe should.

Should we first fix clang then?

They probably shouldn't be using these flags, but I don't know where to diagnose it either. They can properly set the function attributes themselves. The purpose of these flags is to help writing lit tests

We should somehow translate opencl options into user code. I agree these should not be set on library functions. If clang properly set attributes now then this code indeed needs to be removed.

clang and lld do not call the function to do this, which they maybe should.

Should we first fix clang then?

They probably shouldn't be using these flags, but I don't know where to diagnose it either. They can properly set the function attributes themselves. The purpose of these flags is to help writing lit tests

We should somehow translate opencl options into user code. I agree these should not be set on library functions. If clang properly set attributes now then this code indeed needs to be removed.

The user facing flags should set the function attributes. clang can propagate the attributes into the OpenCL library when linked, but we currently aren't using clang to do the linking

rampitec accepted this revision.Dec 10 2019, 9:44 AM

clang and lld do not call the function to do this, which they maybe should.

Should we first fix clang then?

They probably shouldn't be using these flags, but I don't know where to diagnose it either. They can properly set the function attributes themselves. The purpose of these flags is to help writing lit tests

We should somehow translate opencl options into user code. I agree these should not be set on library functions. If clang properly set attributes now then this code indeed needs to be removed.

The user facing flags should set the function attributes. clang can propagate the attributes into the OpenCL library when linked, but we currently aren't using clang to do the linking

I do not think it really should propagate attributes into the library. LGTM.

This revision is now accepted and ready to land.Dec 10 2019, 9:44 AM