Page MenuHomePhabricator

[clang][codegen] Remove the insertion of `correctly-rounded-divide-sqrt-fp-math` fn-attr.
ClosedPublic

Authored by hliao on Fri, Sep 25, 7:32 AM.

Details

Summary
  • -cl-fp32-correctly-rounded-divide-sqrt is already handled in a per-instruction manner to annotate the accuracy required. There's no need to add that fn-attr. So far, there's no backend handling that attr and that option is an OpenCL-specific one, which should not be inserted everywhere.

Diff Detail

Event Timeline

hliao created this revision.Fri, Sep 25, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 25, 7:32 AM
hliao requested review of this revision.Fri, Sep 25, 7:32 AM
hliao updated this revision to Diff 294307.Fri, Sep 25, 7:38 AM

Remove the irrelevant change on .clang-format.

bader added a comment.Mon, Sep 28, 2:22 AM

I'm okay with this change in general. One note though.

There are two aspects of this change:

  1. I agree that cl-fp32-correctly-rounded-divide-sqrt is OpenCL specific and nothing should not be added in other modes.
  2. I have concerns WRT removing the attribute in OpenCL mode. Most of the OpenCL back-ends are out-of-tree and might still rely on a function attribute. In addition to that https://reviews.llvm.org/D22940 handles division part of the requirement, but I don't know if sqrt computation will be handled correctly.

Can we split the patch into two: first to add attribute only in OpenCL language mode and second one removing function attribute in OpenCL mode as well?
In case we need to rollback the patch, I expect no issues with the first part.

hliao updated this revision to Diff 294718.Mon, Sep 28, 8:34 AM

Split the original into 2. This is the first part, which add
correctly-rounded-device-sqrt-fp-math for OpenCL only. The second part will
remove that attribute annotating completely.

bader accepted this revision.Mon, Sep 28, 8:36 AM

Thanks!

This revision is now accepted and ready to land.Mon, Sep 28, 8:36 AM
hliao added a comment.Mon, Sep 28, 8:39 AM

Thanks!

Part 2 is submitted @ https://reviews.llvm.org/D88424

This revision was landed with ongoing or failed builds.Mon, Sep 28, 8:40 AM
This revision was automatically updated to reflect the committed changes.