Could you also update macros and attributes which implemented in https://reviews.llvm.org/D94403 and https://reviews.llvm.org/D94931

LGTM

LGTM

In the mean time I will park this change, and add 'v' implying its subfeatures in clang, since this would catch most cases we expect users to use (I don't think manually enabling features is a common use case). I can then later look at a more detailed/complete fix when enabling features directly with -mattr.

@jrtc27 just let you know I have same concern too, that's one major reason why we don't upstream those extension on GNU toolchain... we are intend to introduce an internal revision number on ELF attribute in near future, e.g. v-ext 0.9.1 / v0p9p1 to prevent compatible issue here.

Note from the last sync up call, it's ok to landing that once review is done without refactor, refactor could be done separately after LLVM 12 release, that's won't be a blocker issue for this patch.

In D95146#2513028, @simoncook wrote:

In D95146#2512871, @craig.topper wrote:

Doesn't this mean that if you only enable zvlsseg, you'll be able to use the instruction in that extension but not the vsetvli instruction that you need to program the VL register?

@craig.topper To be honest, I'm not at all familiar with the v extension or any of the zv* extensions. I wrote this in response to D94931 which says that V should imply zv* but not the other way around. Looking back now at D85069 it looks there was some discussion suggesting which way around it should be, so it could be this patch is unnecessary and breaks stuff. Do you have any better understand of the vector spec as to which way round is correct?

@kito-cheng Similarly, do you know if the correct order of implication/requirements is well defined somewhere. If there are gcc/binutils patches what does it do here?

In D94403#2512232, @asb wrote:

@kito-cheng could you please confirm that this patch handles sub-extensions in the same way GCC does. i.e. -march=rv32izbb0p92 defines __riscv_zbb but NOT __riscv_b?

I believe the behavior has aligned to GCC now.

Add a dedicated test file to demonstrate and verify the ABI would be better.

In D94931#2506156, @simoncook wrote:

In D94931#2506141, @kito-cheng wrote:

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L171

I was thinking about this too and was going to discuss it on Thursday's call. At the very least, even if we don't move the parser, the number of places that extension version numbers are add and used throughout the tools seems to call for it being centralised somewhere.

Just note how current GCC implemented, GCC implement that like implied extension, e.g. V implied Zvamo and Zvlsseg, so __riscv_zvamo is naturally defined when V-ext is enabled.

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

Thanks you implement that on clang, I think it's really great to included that in LLVM 12 release.

RVB 0.93 is an awkward version to me, there is mnemonic conflict which is not resolved during release process since it's kind of too rush, the conflict one is bext in zbe and zbs...

In D94142#2489469, @craig.topper wrote:

It won't be a struct in clang's type system. It's it own special builtin type. I hope we can control the codegen of that type and emit it as multiple allocas/loads/stores. I haven't looked at this yet. Clang can also emit fixed size memcpys of structs which would be broken for this. So we are going to need to customize clang.

This patch removes the existing restriction for this. I've modified
StructType::isSized to consider a struct containing scalable vectors
as unsized so the verifier won't allow loads/stores/allocas of these
structs.

Don't allow function arguments or returns to use structs containing scalable vectors unless they are intrinsics.

Note from gnu toolchain side, GCC will detect bintuils version and behavior during configure time and set different default behavior according the version or feature support test result, but I know clang/llvm doing different way on this part.

64 bit splat on rv32 code gen sequence is LGTM, but I think that's all I can review :P

I was wondering if we could do something like (with a0=lo and a1=hi):

vmv.v.x vX, a1
vsll.vx vX, vX, /*32*/
vor.vx vX, vX, a0

And then do a .vv operation.

We can then optimize for a sign-extended 32-bit value later. What do you think?

Also, it will not handle 64-bit scalars in RV32, but I don't believe that's
actually supported in the spec?

Do you have implement register pair for rv32ifd_zfinx? I didn't saw the related implementation, but I could be wrong since I am not LLVM expert, in case you have implemented, you need a test case for that.

Spec changes has been merged :)

That's LGTM too, just waiting the spec merge :)

I think zv* need to set too, seems like we should have more word about extension is consisted with several sub-extensions on the ELF attribute spec.

Could you add zfh to ELF attribute output?

@MaskRay Thanks, that's first time I know the suffix -SAME :P

ChangeLog:

• Update testcase according to MaskRay's suggestion.

In D89025#2327749, @luismarques wrote:

In D89025#2324334, @khchen wrote:

RISCV supports -mcpu with default empty arch to align gcc's -mtune behavior since clang didn't support -mtune before. But now clang has -mtune, is it a good idea to remove those options? (ex. rocket-rv32/rv64, sifive-7-rv32/64)

If possible that would good, since -mcpu is deprecated (for e.g. x86_64) or unsupported in GCC (for e.g. RISC-V). So doing that would further align Clang with GCC. But I wonder if this might be too problematic, in terms of compatibility.

ChangeLog

• Fix wording in comment
• Add more comment in testcase
• Fix format issue.

If possible that would good, since -mcpu is deprecated (for e.g. x86_64) or unsupported in GCC (for e.g. RISC-V). So doing that would further align Clang with GCC. But I wonder if this might be too problematic, in terms of compatibility.

But this logic is enabled only if CRT_HAS_INITFINI_ARRAY is not defined. Which is controlled by cmake (configuration script). In addition, other architectures (x86, sparc, arm, aarch64, sparc, powerpc) - do allow such fallback option. It just seems strange that RISCV does not have one.

In D87997#2291675, @EccoTheDolphin wrote:

In D87997#2289211, @kito-cheng wrote:

RISC-V LLVM and GCC are default using init_array, curious why we need this?

Actually, yes. You are correct - this one is not needed for ASAN support... My understanding is that this may be needed if we link against some custom runtime. I'll adjust the description of the patch.

RISC-V LLVM and GCC are default using init_array, curious why we need this?

Hi Jun:

GCC part should review at gcc-patch mailing list instead of here, for other files (sanitizer_platform.h, sanitizer_common.h and sanitizer_symbolizer_libcdep.cpp), it's right place to review, but it should using the diff which generated within LLVM source-tree, and GCC will sync libsanitizer after LLVM is accepted.

What about config/riscv/riscv.c? Should I place it here to review?

Hi Jun:

Zvamo implies A-extension.

I think it's require instead of imply?

Upstream didn't support those SiFive specific function attribute, and there is no plan to upstream unless CLIC ratified.
And I guess the attribute name would changed after GCC upstream, at least the prefix of SiFive- would be removed.

Another proposal for -mcpu and -mtune:

Could you add following kind of test:

It seems to me that all remarks have already been addressed. Is there anything holding this patch? For it pretty much LGTM.

Seems like this patch mixed with LTO related changes? Could you clean it up?

In addition, I think my testcase is so weird and it does not make sense there are different isa extension are used in the same compilation unit...

Add 2 test cases.

@MaskRay Thanks, I'll add test case soon :)

Changes:

• Update test/MC/RISCV/rvi-pseudos-invalid.s

Changes:

• Fix parseBareSymbol with a constant symbols.
• Rebase to D55325

@jrtc27 feel free to upload your one, it's just a 5 minute quick patch :P

Changes:

• Update testcase, R_RISCV_RELAX won't bind with symbol to fit binutils's behavior.

Changes:

• Add missing tests

Okay, I'll take a look :)

Changes:

• Merge emitLoadLocalAddress/emitLoadSymbol/emitStoreSymbol
• Fix wrong operand type and syntax for floating point load instruction.

rv32e arch and ilp32e ABI is decoupling in GCC, that's mean rv32i with ilp32e is possible, so I would suggest separate two thing.

Hi Ana:

GCC using 5 as threshold to decide using jump table or not for RISC-V and LLVM using 4.

Changes:

• Add test.

Changes:

• Move declaration of RelaxCandidate.
• Add comment.
• Update testcase

Hi Roger:

Changes:

• Rebase.

Changes:

• Rename functions
  • emitLoadSymbolAddress -> emitLoadSymbolAddressHi
  • emitLoadWithSymbol -> emitLoadSymbol
  • emitStoreWithSymbol -> emitStoreSymbol
• Return shouldForceImediateOperand to true for new pseudo instructions.
• Add negative test

• Add default case in switch.

Rearrange order for testcase.

Changes:

• Update testcase for rv32i-aliases-invalid.s and rv64i-aliases-invalid.s
• Fix rv32i-invalid.s

Changes:

• Add addw, sllw, srlw and sraw including testcase.
• Add comment in testcase.

Update testcase.