This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use original mask for restoring the original sign instead of from setcc
AbandonedPublic

Authored by Jim on Jul 20 2023, 6:26 PM.

Details

Summary

It should use the mask that for fabs operation to restoring the original
sign instead of generated from setcc operation that only mask the value need
to be converted.

Diff Detail

Event Timeline

Jim created this revision.Jul 20 2023, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 6:26 PM
Jim requested review of this revision.Jul 20 2023, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 6:26 PM
craig.topper added inline comments.Jul 20 2023, 6:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2574

No underscores in variable names.

Jim updated this revision to Diff 542751.Jul 20 2023, 8:21 PM

Rename variable

Could you provide an example why we need to use the old Mask for copysingn?

Jim marked an inline comment as done.Jul 20 2023, 8:21 PM
Jim added a comment.Jul 20 2023, 8:40 PM

Could you provide an example why we need to use the old Mask for copysingn?

I do not have C code or LLVM IR example.

During lowering rounding operation, it is converted to the absolute value with the mask that is from vp (or all ones mask for unmasked),
and compare with the largest integer that can be represented exactly. This comparision generates new mask that is used to only convert
the value less than the largest integer. The end, we copy the sign back to the value that should use the old mask the same with fabs operation.

Jim added inline comments.Jul 20 2023, 8:48 PM
llvm/test/CodeGen/RISCV/rvv/ceil-vp.ll
46

From this change vfsgnj.vv v8, v9, v8, v0.t -> vfsgnj.vv v8, v9, v8, v0 is from vmflt.vf (compared with the largest interger).
It should copy sign back for all active element like fabs operation (vfabs.v v9, v8) also for all active element.

During lowering rounding operation, it is converted to the absolute value with the mask that is from vp (or all ones mask for unmasked),
and compare with the largest integer that can be represented exactly. This comparision generates new mask that is used to only convert
the value less than the largest integer. The end, we copy the sign back to the value that should use the old mask the same with fabs operation.

I think those elements inactive for SetccMask are not changed during the truncation and we don't care them.

During lowering rounding operation, it is converted to the absolute value with the mask that is from vp (or all ones mask for unmasked),
and compare with the largest integer that can be represented exactly. This comparision generates new mask that is used to only convert
the value less than the largest integer. The end, we copy the sign back to the value that should use the old mask the same with fabs operation.

I think those elements inactive for SetccMask are not changed during the truncation and we don't care them.

I don't think there was a bug here either. The passthru value for the copysign is the original source before the absolute value.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2623

All of the operations that produced Truncated before this are using masked agnostic policy. Mask here is now a superset of SetccMask. So far all the bits in Mask that aren't set in SetccMask, we are copying the sign from elements in Src to "agnostic" elements in Truncated.

Jim abandoned this revision.Jul 20 2023, 10:07 PM

During lowering rounding operation, it is converted to the absolute value with the mask that is from vp (or all ones mask for unmasked),
and compare with the largest integer that can be represented exactly. This comparision generates new mask that is used to only convert
the value less than the largest integer. The end, we copy the sign back to the value that should use the old mask the same with fabs operation.

I think those elements inactive for SetccMask are not changed during the truncation and we don't care them.

I don't think there was a bug here either. The passthru value for the copysign is the original source before the absolute value.

I don't consider that copysign back only for converted value. Inactive element in SetccMask is still not changed in the source.