Thanks @lewis-revill this LGTM.

I have a question about the meaning of the vlen operand so I want to double-check here.

In D45181#1488084, @luismarques wrote:

@rogfer01 Are you still looking into this? My efforts to fix the problem with the debug information would also touch this. How do you want to proceed?

I've been looking into this and I have some observations, but not a satisfactory solution so far.

James, Alex: Thanks a lot for the comments. I'll look into what I can do here.

I don't like resurrecting old reviews. This is just a warning in advance of a future issue we will have with the RISC-V backend when exception handling is enabled (it is not yet upstream).

Thanks a lot for the review Alex!

ChangeLog:

• Improve comment with wording kindly suggested by Alex Bradbury

Lewis, Alex: thanks for the review.

ChangeLog

• Remove dead phi in the test

Thanks for the update Alex, certainly vararg.ll is a better test to update than my earlier suggestions.

Hi Luis,

Thanks Alex. I will commit it shortly.

Thanks a lot for this Alex.

Looks good to me, just a minor suggestion above.

@asb in D59298 I call riscv::getRISCVABI for ClangAs, does it make sense to make the same change for Clang here?

Looks sensible to me.

@jmorse pr40427.ll uses x86-specific bits in the CHECK lines and is causing failures in builds that do not target by default x86 (e.g. http://lab.llvm.org:8011/builders/clang-ppc64le-linux-lnt/builds/16847 )

Hi @lewis-revill, thanks for the patch.

Ah, I think what happens is that we should not emit such patterns without the presence of the F extension: these patterns are ultimately selected as instructions in F. Does this make sense?

Hi Alex,

Hi Alex,

In D54143#1325501, @lewis-revill wrote:

Rebased and updated to use and expand PseudoLLA for PC-relative addressing.

Minor comment above but other than that this looks good to me too.

The build issue was fixed in rL348783

Hi all,

Not needed for now.

Upstream recently amended the ABI spec so it looks to me this is not going to be needed.

Thanks for the review @gbedwell @MaskRay

My only problem with that approach is that it seems wrong to expand PseudoLLA the same way I am expanding PseudoAddrPCRel, IE allowing the AUIPC operand to be decided by codegen.

Ah of course. I hadn't thought of that case. I guess we don't expect symbols to end in @plt as part of their name, do we?

Would it make sense to add an operand that allows a bare symbol or bare symbol + @plt and use it only in CALL and TAIL instead of allowing identifier@plt everywhere?

Yeah, upstream suggested axing the list of assembly operators from the psABI doc as it's unnecessary duplication that gets out of date. Volunteers?...

Ah, thanks @asb definitely I was looking at the wrong place!

This patch is a very reasonable one: it is a bit weird we can't expand la (under -fPIC) because R_RISCV_GOT_HI20 relocation does not have any associated syntax.

Hi, thanks for the patch. I don't think this usage is wrong so we don't have to change it.

Thanks both for the prompt answer!

the test MC/Disassembler/RISCV/invalid-instruction.txt contains this case

In my downstream I have a single RISCVISD::WRAPPER_PIC and then I use a target flag to remember whether later we want just PCREL (lla) or GOT based on what shouldAssumeDSOLocal returns.

On hold for now.

Thanks Luis. I merged your earlier change D54034 into my downstream compiler just to find that frem was missing (triggered by a downstream test).

Here is an example of two dvl invocations, which compute the same result:

a) llvm_dvl_fadd.v256f64(%x, %y, <full mask>, 13)
Since 13 < 32, the hardware will only issue 1 operation to its SIMD execution units. The occupation is thus something like 13/32 ~ 40%.

b`) llvm_dvl_fadd.v256f64(%x, %y, <mask with first 13 bits set>, 256)
Since the DVL is 256, the hardware will issue 8 operations to its SIMD units. However, only the first 13 elements are relevant leading to an occupation of 13/256 ~ 5%.

I agree with @fpetrogalli here that there is some overlap between a "dynamic vector length" i32 %dvl and a mask of the form %m = <i1 0, ..., i1 0, i1 1, ..., i1 1> (or the reverse, if the lanes are to be interpreted in the other direction) where %dvl = llvm.ctpop(%m). As Francesco, puts it, we can always construct %m from %dvl. Perhaps I'm wrong, but I think in the context of strip-mined loops or whole function vectorisation, more elaborated masks that might arise due to control flow would always be subsumed by %m (i.e. will have a strict subset of the lanes enabled by %dvl).

Hi @efriedma thanks a lot for this improvement.

Yes, the changes in this RFC are compatible with a physical simdlen that is unknown at compile time.

Hi Simon,

Sorry I missed the RFC. After having read it I agree with you. Sorry for the noise.

rv64i has a few specific *w instructions that we probably want to test them too. For instance,

The updated testcase does not fail locally anymore.

Thanks I will do it shortly.

I can commit it.

Thanks for the review!

on a second thought, "InstalledDir" only appears with -v, and is still too fragile (e.g if installed dir name also contains checked strings)

Sure, I didn't mean to do that broader change here. Apologies if it read that way.

Thanks for reopening this @kristina.