This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fix extract_vector_elt on i1 at idx 0 being inverted
ClosedPublic

Authored by luke on May 3 2023, 2:29 AM.

Details

Summary

It looks like the intention here is to truncate a XLenVT -> i1, in
which case we should be emitting snez instead of sneq if I'm understanding
correctly.

Diff Detail

Event Timeline

luke created this revision.May 3 2023, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 2:29 AM
luke requested review of this revision.May 3 2023, 2:29 AM
jacquesguan accepted this revision.May 3 2023, 7:04 PM

Thanks to fix this, LGTM.

This revision is now accepted and ready to land.May 3 2023, 7:04 PM
frasercrmck accepted this revision.May 3 2023, 11:28 PM

LGTM. Should this fix be backported to 16.0.X?

This revision was automatically updated to reflect the committed changes.
rogfer01 added a comment.EditedMay 10 2023, 1:00 AM

Hi, we are observing failures in the llvm testsuite (when compiling with -march=rv64gcv) and git bisect pointed here

Failed Tests (2):
  test-suite :: SingleSource/Regression/C/gcc-c-torture/execute/GCC-C-execute-pr53645-2.test
  test-suite :: SingleSource/Regression/C/gcc-c-torture/execute/GCC-C-execute-pr53645.test

can anyone else reproduce this issue too?

craig.topper added a comment.EditedMay 10 2023, 3:04 PM

I think SETEQ was correct before. We're checking the result of vfirst which can return a value between [-1, VL). We want to know specifically if bit 0 is set. There are 3 cases to consider. If bit 0 is set, the result of the vfirst will be 0. If bit 0 is not set but some other bit is, the result of the vfirst will be >0. The third case is that bit 0 isn't set and no other bits are set. In that case the vfirst will return -1.

The first case is the only case we should return 1. So we should be checking that the result of the vfirst is equal to 0, the original code before this patch.