Fixed Instruction Selection Patterns for CoreV SIMD Insert and SIMD E…

[adeel10x]

…xtract Instructions

[ChunyuLiao]

can you add a crash testcase?

[adeel10x]

can you add a crash testcase?
yes, sure.

[adeel10x]

can you add a crash testcase?

done

[ChunyuLiao]

I don't know why your patch isn't out triggering CI. maybe @PaoloS02 help approve to trigger CI. I don't have permission.

[PaoloS02]

Sorry for the delay, we've had the chance to have a hard look at the patch and from what I see in the patch we are silently truncating the parameter to 6 bits. I don't think that's very developer friendly. It would technically be better to let the compiler crash if someone passes a number that is not 6 bits as there wouldn't be an instruction to match that. A compiler crash is not a good thing either so it is better to error earlier.
What we need is the validation of the constant argument in clang, when the clang builtin is called so that clang errors before we get to instruction selection in LLVM and things blow up. You can add cases for the insert and extract instructions in a CORE- V version of CheckRISCVBuiltinFunctionCall in clang/lib/Sema/SemaChecking.cpp. You can see an example of that with the __builtin_riscv_aes32dsi_32 builtin and similar. This will actually be useful also for other simd intrinsics, like add.h, cplxmul*... so another patch will be needed.

Would it be possible to split this patch so that here you correct the sign of the operand of insert/extract in the pattern matching as you are already doing but without the truncation LO6 and in another patch you add the semantic checks in clang for similar simd builtins and some tests trigger such semantic checks?
Of course the cases with 255 in the codegen tests should not be necessary any more after the change.

[adeel10x]

Sorry for the delay, we've had the chance to have a hard look at the patch and from what I see in the patch we are silently truncating the parameter to 6 bits. I don't think that's very developer friendly. It would technically be better to let the compiler crash if someone passes a number that is not 6 bits as there wouldn't be an instruction to match that. A compiler crash is not a good thing either so it is better to error earlier. What we need is the validation of the constant argument in clang, when the clang builtin is called so that clang errors before we get to instruction selection in LLVM and things blow up. You can add cases for the insert and extract instructions in a CORE- V version of CheckRISCVBuiltinFunctionCall in clang/lib/Sema/SemaChecking.cpp. You can see an example of that with the __builtin_riscv_aes32dsi_32 builtin and similar. This will actually be useful also for other simd intrinsics, like add.h, cplxmul*... so another patch will be needed.

Would it be possible to split this patch so that here you correct the sign of the operand of insert/extract in the pattern matching as you are already doing but without the truncation LO6 and in another patch you add the semantic checks in clang for similar simd builtins and some tests trigger such semantic checks? Of course the cases with 255 in the codegen tests should not be necessary any more after the change.

Yes, sure.

[adeel10x]

Hi, @PaoloS02 @ChunyuLiao I have reverted the unwanted changes and fixed the sign of immediate operand in the instruction selection pattern. For the generation of clang errors, I have done a separate PR just like you mentioned.

[PaoloS02]

Looks good! I'll now review the related PR #93