This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add tests for memory constraint A
ClosedPublic

Authored by wangpc on Sep 18 2023, 10:25 PM.

Details

Summary

We should not optimize it in D158062. This adds the test coverage.

And unneeded attributes nonnull and inbounds are removed.

Diff Detail

Event Timeline

wangpc created this revision.Sep 18 2023, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2023, 10:25 PM
wangpc requested review of this revision.Sep 18 2023, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2023, 10:25 PM
asb accepted this revision.Sep 19 2023, 4:19 AM

LGTM, one minor nit. You're consistent with other tests in this file with using nonnull and inbounds, but I'd typically drop any attributes that don't alter the output of the test.

This revision is now accepted and ready to land.Sep 19 2023, 4:19 AM
wangpc updated this revision to Diff 557031.Sep 19 2023, 4:49 AM

Remove nonnull and inbounds.

wangpc edited the summary of this revision. (Show Details)Sep 19 2023, 4:49 AM
This revision was landed with ongoing or failed builds.Sep 19 2023, 4:56 AM
This revision was automatically updated to reflect the committed changes.

Read the banner at the top of every Phabricator page. New revisions should be submitted via GitHub.

asb added a comment.Sep 19 2023, 7:59 AM

Read the banner at the top of every Phabricator page. New revisions should be submitted via GitHub.

Yes, but I think this was posted here rather than as a PR as it's a pre-requisite for a patch that had already started review on Phabricator. So it seems reasonable enough in this instance.

Read the banner at the top of every Phabricator page. New revisions should be submitted via GitHub.

Yes, but I think this was posted here rather than as a PR as it's a pre-requisite for a patch that had already started review on Phabricator. So it seems reasonable enough in this instance.

Oh, I thought I had checked for the Stack button as I guessed that might be the case, but seems I managed to miss it. My bad.