Page MenuHomePhabricator

[OpenCL] Minor cleanup to access attributes on images
ClosedPublic

Authored by joey on Nov 15 2016, 7:05 AM.

Details

Reviewers
aaron.ballman
Summary

Use the semantic spelling (an enum) rather than a string, to determine what access qualifier is used.

Diff Detail

Repository
rL LLVM

Event Timeline

joey updated this revision to Diff 77995.Nov 15 2016, 7:05 AM
joey retitled this revision from to [OpenCL] Minor cleanup to access attributes on images .
joey updated this object.
joey set the repository for this revision to rL LLVM.
joey added a subscriber: cfe-commits.
aaron.ballman added inline comments.
lib/Sema/SemaType.cpp
1211–1212

Since we're updating this, can you change attrs to Attrs, and maybe make it a const AttributeList *?

1224

The caller can no longer tell the difference between a real-only OpenCL access attribute and no OpenCL access attribute. I know that this was the effective behavior of the original code, but that's specific to the current use case, so I think this change is a tiny regression in the semantics.

1629

Just checking, but, has this situation already been diagnosed elsewhere?

joey updated this revision to Diff 78017.Nov 15 2016, 9:24 AM

Fixed a latent infinite loop bug in 'getImageAccess', it was dereferencing Attrs, instead of Next.

joey marked an inline comment as done.Nov 15 2016, 9:25 AM
joey added inline comments.
lib/Sema/SemaType.cpp
1224

I renamed this function.

1629

Maybe I should actually replace this with an llvm_unreachable?

aaron.ballman accepted this revision.Nov 15 2016, 9:32 AM
aaron.ballman edited edge metadata.

LGTM, with one minor nit.

lib/Sema/SemaType.cpp
1629

I would remove the default label entirely; the switch is now fully covered, so if we remove the label, we can get a diagnostic if a new spelling is added and we forget to update this switch.

This revision is now accepted and ready to land.Nov 15 2016, 9:32 AM
joey closed this revision.Nov 16 2016, 3:53 AM

Committed as r287100.

test/SemaOpenCL/access-qualifier.cl
71

This was an unintentional test change, and was not committed.