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.

Oh this is great! Scales much better than my earlier hack. Thanks a lot @asb !

If I'm understanding correctly, the problem here is that the lui has to have the address of the auipc as a parameter.

ChangeLog:

• Align syntax

Thanks for the prompt review @asb!

Thanks for the review @asb

ChangeLog

• Add test for ConstantPools

Thanks for the update @kito-cheng . Some more comments inline as the change is propagating a mistake of mine I introduced in rL339314 (fixed in rL339654).

Hi, thanks a lot for the patch. A few comments inline.

Thanks for the review @asb . I will commit soon.

Thanks @asb for the review!

Fixed in rL338341

Thanks a lot for the review @asb !

Ah, I didn't notice it was exactly the same. You're right no need to add it again.

Thanks a lot for the patch @pftbest!