Page MenuHomePhabricator

[InstCombine] Update valueCoversEntireFragment to use TypeSize
ClosedPublic

Authored by peterwaller-arm on Nov 19 2020, 10:10 AM.

Details

Summary
  • Update valueCoversEntireFragment to use TypeSize.
  • Add a regression test.
  • Assertions have been added to protect untested codepaths.

Diff Detail

Event Timeline

fpetrogalli created this revision.Nov 19 2020, 10:10 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 19 2020, 10:10 AM
fpetrogalli requested review of this revision.Nov 19 2020, 10:10 AM

I updated the RUN lines of the .ll test to prevent the failure at https://reviews.llvm.org/harbormaster/unit/view/196899/.

FYI: I am trying to figure out this failure that does not appear on my machine: https://reviews.llvm.org/harbormaster/unit/view/196974/

This last change is trying to remove the failure at https://reviews.llvm.org/harbormaster/unit/view/196974/
I haven’t been able to reproduce such failure on my dev machine, so I just changed the RUN lines in the test (added -c, used 2>%t) to see if the bot is going to be happy with the new invocation.

I have removed the C test, as the LL test is enough to test the changes I have done in Local.cpp.

I can confirm that with this patch, the release note ton'r raise any warnings when compiling with debug info:

frapet01@man-08:~/projects/upstream-clang/build-clang$ cat ../release-notes.c
#include <arm_sve.h>

void VLA_add_arrays(double *x, double *y, double *out, unsigned N) {
  for (unsigned i = 0; i < N; i += svcntd()) {
    svbool_t Pg = svwhilelt_b64(i, N);
    svfloat64_t vx = svld1(Pg, &x[i]);
    svfloat64_t vy = svld1(Pg, &y[i]);
    svfloat64_t vout = svadd_x(Pg, vx, vy);
    svst1(Pg, &out[i], vout);
  }
}
frapet01@man-08:~/projects/upstream-clang/build-clang$ ./bin/clang --target=aarch64-gnu-linux -march=armv8-a+sve -c -g -S -O3 -o /dev/null ../release-notes.c
frapet01@man-08:~/projects/upstream-clang/build-clang$

When the patch will end up into a nightly build of compiler explorer, the compilation at this link should not raise any warnings: https://godbolt.org/z/zebzKM

fpetrogalli retitled this revision from [SVE] Remove warnings from release notes example on SVE ACLE. to [SVE] Remove warning from debug info on scalable vector..Nov 20 2020, 7:04 AM
fpetrogalli edited the summary of this revision. (Show Details)

This patch needs to be retitled to what this is actually doing: changing the getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize instead of unsigned.
It would be even better if you can split those up into two patches with separate tests for each one of them.

clang/test/CodeGen/aarch64-sve-acle-rel-note.c
1 ↗(On Diff #306643)

I'm not entirely sure if it is acceptable to have a clang test that generates assembly, but in any case I think this test needs to be added in a separate patch.

llvm/lib/IR/IntrinsicInst.cpp
56 ↗(On Diff #306643)

Change back to auto ?

llvm/lib/Transforms/Utils/Local.cpp
1345

Should the scalable flag match? Same question for all the other cases in this patch.

fpetrogalli marked an inline comment as done.Nov 20 2020, 8:39 AM

Thank you for the review @sdesmalen

  1. I have replied to the comment about auto with my reason for removing it.
  2. I removed the C test, as the LL file is enough.
  3. The third comment requires more investigation, I'll get back to you to see the use of the TypeSize comparisons.

Thank you!

llvm/lib/IR/IntrinsicInst.cpp
56 ↗(On Diff #306643)

I'd like to keep this, because it seems to me the use of auto in this case didn't fit in the cases mentioned in https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable. There is no direct type deduction from the context around the initialization of the variable Fragment.

fpetrogalli edited the summary of this revision. (Show Details)

I have added assertions around before TypeSize comparisons where it made sense to do so, but not in the lambda used in the sort invocation.

fpetrogalli marked an inline comment as done.Nov 20 2020, 9:33 AM
fpetrogalli added inline comments.
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
589 ↗(On Diff #306684)

We don't need to check whether the scalable flag matches here because we are sorting a container that (potentially) have both types of vectors.

Rebase on top of D92020. NFC.

Add comment to DbgVariableIntrinsic::getFragmentSizeInBits(). NFC

fpetrogalli edited the summary of this revision. (Show Details)Nov 24 2020, 6:16 AM

Hi @sdesmalen

I have extracted D92020 to implement only the change of interface for AllocaInst::getAllocationSizeInBits.

I wanted to extract also the interface change in DbgVariableIntrinsic::getFragmentSizeInBits() but such change would require also to change valueCoversEntireFragment: essentially, this is already what I am doing in this patch.,

This patch needs to be retitled to what this is actually doing: changing the getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize instead of unsigned.

Given that the only change resulting from this patch is removing the warning, I think it makes sense to keep the summary of the patch as it is.

I have added a comment in DbgVariableIntrinsic::getFragmentSizeInBits() explaining that to get full support for scalable types in DbgVariableIntrinsic::getFragmentSizeInBits() we should update the DIVariable::getSizeInBits to carry the scalable flag for scalable variables. I am happy to continue in that direction (it seems an extensive change with quite a few implications), I just wanted to double check whether 1. you agree on the need to change DIVariable::getSizeInBits to privide TypeSize info, and 2. if you think if you are happy for the warning fix (this patch) to go in before the DIVariable::getSizeInBits interface change, or 3. do you see a third way? :)

Cheers,

Francesco

Reverted getFragmentSizeInBits to return an integral type and not TypeSize, because scalable variables cannot be part of structs/arrays - hence they cannot be pointed by fragments.

joechrisellis commandeered this revision.Dec 2 2020, 1:45 AM
joechrisellis edited reviewers, added: fpetrogalli; removed: joechrisellis.

I am finishing up this patch. 😄

Reduce debug info in debug-declare-no-warnings-on-scalable-vectors.ll.

Please rename debug-declare-no-warnings-on-scalable-vectors.ll to something different, because these 'warnings' are only temporary and will be replaced by errors in the future. Having 'no warnings' in the name of the test name seems wrong from that perspective.

This patch needs to be retitled to what this is actually doing: changing the getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize instead of unsigned.

Given that the only change resulting from this patch is removing the warning, I think it makes sense to keep the summary of the patch as it is.

Given that the generated warnings are temporary and only a side-effect of the implicit conversion 'hack', and thus not a feature or design choice, I still think the patch needs to be retitled to better phrase what it does, i.e. change getTypeAllocationSizeInBits to return TypeSize instead of unsigned.

Address @sdesmalen's comments.

  • Update commit message to better reflect the changes in this patch.
  • Rename debug-declare-no-warnings-on-scalable-vectors.ll to dbg-info-scalable-typesize-warning.ll.
joechrisellis retitled this revision from [SVE] Remove warning from debug info on scalable vector. to [SVE] Change getTypeAllocationSizeInBits to return TypeSize.Dec 2 2020, 7:39 AM
joechrisellis edited the summary of this revision. (Show Details)
peterwaller-arm commandeered this revision.Dec 14 2020, 8:47 AM
peterwaller-arm edited reviewers, added: joechrisellis; removed: peterwaller-arm.

I've taken over the differential.

Simplify things a bit more, remove an untested remnant created from before the patch split.

peterwaller-arm retitled this revision from [SVE] Change getTypeAllocationSizeInBits to return TypeSize to [InstCombine] Update valueCoversEntireFragment to use TypeSize.
peterwaller-arm edited the summary of this revision. (Show Details)

Attempting to update commit message with arc diff --verbatim.

sdesmalen added inline comments.Jan 4 2021, 12:40 AM
llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
14

After your changes, what line causes this warning to be emitted? Does this test also pass with asserts enabled?

peterwaller-arm added inline comments.Jan 4 2021, 2:25 AM
llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
14

It's a -NOT check, so it's not being emitted. Yes, the test passes even with asserts enabled.

I assume the question is motivated by putting the check next to the line which causes the warning(?).

Update for review comment.

  • Generalize WARN-NOT to disallow any warnings, not just those with specific text.
peterwaller-arm marked an inline comment as done.Jan 5 2021, 8:39 AM
peterwaller-arm added inline comments.
llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll
14

Discussed out of band. Sander's interest was to disallow any warning output, as with other SVE ASM-NOT tests in the clang directory.

this lgtm with all other reviewer's concerns addressed.

This revision is now accepted and ready to land.Jan 6 2021, 5:55 AM
This revision was landed with ongoing or failed builds.Jan 6 2021, 9:16 AM
This revision was automatically updated to reflect the committed changes.