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

Mon Jun 20 08:27:11 PDT 2022

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Despite the comments above, the purpose of this patch remains unclear.

Per the draft spec, the relevant wording is:
"These instructions execute as a regular load except that they will only take a trap caused by a synchronous exception
on element 0. If element 0 raises an exception, vl is not modied, and the trap is taken. If an element > 0 raises an
exception, the corresponding trap is not taken, and the vector length vl is reduced to the index of the element that would
have raised an exception."

Working through the scenario in this patch with the destination being null, the expected result is for a trap to be generated (provided null is unmapped of course), and VL not to be modified.  In order for this change to make any difference in runtime behavior, the value passed must be null (or otherwise guaranteed to fault).  It seems very odd to me that we are modifying code which only runs after an instruction which is guaranteed to fault.  Is there an assumed runtime here which is e.g. restarting execution?

Presumably, the actual IR instruction returns the unmodified VL in the faulting first access case.  (If it doesn't, that's a bug we should fix.)

The last point, and it's a critical one, is that the outparam for new_vl does not have to be null if dest is.  Given that, I think the entire prior discussion on motivation here is off base.  Unless you can point to something in the intrinsic docs which says *explicitly* that the new_vl param to the intrinsic can be null when the dest is known to fault, I think we should strongly reject this patch.  Even if you can, I think we should first ask if that's a bug in the intrinsic spec.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126461