This is an archive of the discontinued LLVM Phabricator instance.

[15/15][Clang][RISCV][NFC] Set data member under Policy as constants
ClosedPublic

Authored by eopXD on Jan 15 2023, 9:57 AM.

Details

Summary

The object is now correct by construction.

This is the 15th commit of a patch-set that aims to change the default policy
for RVV intrinsics from TAMU to TAMA.

Please refer to the cover letter in the 1st commit (D141573) for an
overview.

Depends on D141793.

Diff Detail

Event Timeline

eopXD created this revision.Jan 15 2023, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2023, 9:57 AM
eopXD requested review of this revision.Jan 15 2023, 9:57 AM
craig.topper added inline comments.Jan 19 2023, 12:34 AM
clang/include/clang/Support/RISCVVIntrinsicUtils.h
95–96

Should this be a class instead of struct so the members are private?

eopXD updated this revision to Diff 490400.Jan 19 2023, 1:02 AM

Update code: Address comment from Craig.

eopXD updated this revision to Diff 490407.Jan 19 2023, 1:15 AM
eopXD marked an inline comment as done.

Bump CI.

This revision is now accepted and ready to land.Jan 19 2023, 10:48 PM
This revision was landed with ongoing or failed builds.Jan 24 2023, 8:38 AM
This revision was automatically updated to reflect the committed changes.
DavidSpickett added a subscriber: DavidSpickett.EditedJan 24 2023, 9:47 AM

Hi! Comparing the changes for https://lab.llvm.org/buildbot/#/builders/17/builds/33129 and https://lab.llvm.org/buildbot/#/builders/174/builds/17069 I think you've managed to hit a crash in the host compiler we (Linaro) are using for our bots. Which is the 15.0.0 release.

I will handle it, perhaps I can reduce it down to some pattern 15.x doesn't like but either way I don't think this change is at fault.

eopXD added a comment.EditedJan 26 2023, 5:33 AM

Hi @dyung and @DavidSpickett,

Thank you for checking this out and sorry for the late reply (Chinese New Year holiday zone right now in Taiwan). Let me look into it now.

eopXD reopened this revision.Jan 26 2023, 5:48 AM

Reopening the revision to try land this correctly.

This revision is now accepted and ready to land.Jan 26 2023, 5:48 AM
eopXD updated this revision to Diff 492420.Jan 26 2023, 5:54 AM

Rebase to latest main.

eopXD added a comment.Jan 26 2023, 6:21 AM

This commit seems to cause riscv_vector_builtin_cg.inc to explode into a 630K file, which causes the out-of-memory build fail. Confirmed that this is the commit that is causing the error.

Nice!

Glad you spotted that, I have had creduce running and getting nowhere fast for a day or so. Sounds like it would never have found anything minimal.

eopXD added a comment.Jan 26 2023, 6:28 AM

Nice!

Glad you spotted that, I have had creduce running and getting nowhere fast for a day or so. Sounds like it would never have found anything minimal.

Indeed! Thank you for your time again :)

eopXD updated this revision to Diff 492437.Jan 26 2023, 7:02 AM

Update code. The removal of PolicyAttrs.IsUnspecified = false; under

void RVVIntrinsic::updateNamesAndPolicy(bool IsMasked, bool HasPolicy,
                                        std::string &Name,
                                        std::string &BuiltinName,
                                        std::string &OverloadedName,
                                        Policy &PolicyAttrs) {

/* ... */
  if (PolicyAttrs.isUnspecified()) {
  }

is incorrect becasue the == operator of the Policy object still uses it.
So repeating builtin-s were created (becasue repeating ones are not recognized
as duplicates), causing riscv_vector_builtin_cg.inc to explode.

The updated patch now only adds const qualifier to TailPolicy and
MaskPolicy.

eopXD added a comment.Jan 26 2023, 7:06 AM

Re-commiting this now that the problem is resolved.

This revision was landed with ongoing or failed builds.Jan 26 2023, 7:06 AM
This revision was automatically updated to reflect the committed changes.