[PATCH] D126461: [RISCV] Extract and store new vl of vleff iff destination isn't null

Wang Pengcheng via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 8 23:35:19 PDT 2022


pcwang-thead added a comment.

In D126461#3566626 <https://reviews.llvm.org/D126461#3566626>, @khchen wrote:

> IMO, if I'm an user, I would not expected intrinsic function will generate the condition code to impact the performance, maybe we need to raise a issue in rvv-intrinsic-doc.
> maybe another way is adding a note in intrinsic document to address that the vl could not be a null pointer.

I understand your concern about impact on performance and I added a test to show that the comparison and branch instruction can be optimized if destination is known to be not null (which is common in most scenarios using vleff).
For example:

  size_t new_vl;
  v=vleff(..., &new_vl, vl);
  //usage of new_vl.

When doing `instsimplify`, LLVM knows `new_vl` won't be null. Comparison is evaluated to be true and branch instruction will be changed to unconditional branch (which will be simplified by `simplifycfg`).
So I think there is no impact on performance for most scenarios. For some cases that we don't know whether destination is null or not (like using destination passed by argument in `vleff.c`, but I think it is not normal usage and IPO may help), it is a potential risk that we may end in program crashing if we don't do null pointer checking.

> How about the segment load? Does it make sense to add null pointer checking for all argument v0~vN?

That's a good point and I have thought it before. Here is my point but we can have more discussion.
I think we don't need null pointer checking for non-tuple segment loads. One of the reason is that we have said (but not documented?) that pointers can't be null in https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/95. Besides, I think we should treat the output of segment loads as whole structure though we have non-tuple intrinsics for now. The reason why we add null pointer checking for vleff is because the outputs of vleff are of two different types and we just want to ignore the output new vl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126461/new/

https://reviews.llvm.org/D126461



More information about the cfe-commits mailing list