In D140460#4172085, @craig.topper wrote:

LGTM

In D141899#4065375, @nikic wrote:

In D141899#4061237, @zixuan-wu wrote:

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

1. Remove X86amx type in IR totally. (what I am doing now)
2. Without removing X86amx type in IR, just upgrade the x86amx type to target extension and also upgrade bitcast llvm instruction to intrinsic(required). It also includes changing the testcase to target extension type.

I believe the right option is:

3. Remove x86_amx type from the (in-memory) IR representation, but support an auto-upgrade for bitcode only.

We do need bitcode auto-upgrade support as a matter of policy, and we shouldn't support both type representation at the same time, that would defeat the point of the change.

In D141899#4061174, @LuoYuanke wrote:

I think target extension type is nice, if it is introduced 2 years ago I would vote for it. However my concern is the compatibility issue as I explained. We need to be compatible to the IR that built by previous compiler, and be compatible to the 3rd party software that based on the x86_amx type. I can't predict more risks for now if we replace an LLVM IR type, but I believe there is big risk hidden.

Due to bitcode auto-upgrade, compatibility with old IR is retained. As long as we avoid some of the API changes here, the impact on downstream code should be fairly minimal. (Though as already pointed out, downstream impact generally doesn't figure into LLVM design decisions anyway.)

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

In D141899#4058173, @LuoYuanke wrote:

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

Gentle pin..

In D135202#4042297, @craig.topper wrote:

In D135202#4042268, @pengfei wrote:

In D135202#4040194, @jcranmer-intel wrote:

In D135202#4038774, @zixuan-wu wrote:

I'd like to enable the backend infra and propose draft later. To better demonstrate and enable backend target-independent code completely, I'd like to demonstrate with x86_amx that changing x86_amx type to target extension type and make all test passed, if nobody objects. @jcranmer-intel @LuoYuanke @craig.topper @pengfei

I have no objections to this; indeed, my original proposal for target extension types included x86_mmx and x86_amx as types that could be implemented as target extension types. However, I'm not involved with the x86 backend, so I must defer to the proper codeowners there as to whether or not to do this.

I think it's more appropriate to start with x86_fp80 and x86_mmx. I'd expect there will be a more general matrix representation for the AMX type, e.g., TLX, <8x8xi8> etc. rather than limit it to target extension type, though I'm not against to do prototype for it.

Aren't both x86_fp80 and x86_mmx recognized by InstCombine? They aren't truly opaque.

In D135202#4042268, @pengfei wrote:

In D135202#4040194, @jcranmer-intel wrote:

In D135202#4038774, @zixuan-wu wrote:

I'd like to enable the backend infra and propose draft later. To better demonstrate and enable backend target-independent code completely, I'd like to demonstrate with x86_amx that changing x86_amx type to target extension type and make all test passed, if nobody objects. @jcranmer-intel @LuoYuanke @craig.topper @pengfei

I have no objections to this; indeed, my original proposal for target extension types included x86_mmx and x86_amx as types that could be implemented as target extension types. However, I'm not involved with the x86 backend, so I must defer to the proper codeowners there as to whether or not to do this.

I think it's more appropriate to start with x86_fp80 and x86_mmx. I'd expect there will be a more general matrix representation for the AMX type, e.g., TLX, <8x8xi8> etc. rather than limit it to target extension type, though I'm not against to do prototype for it.

I don't have an example backend that goes through GlobalISel or SelectionDAG yet; I've been focusing my effort on the production SPIR-V backend which translates to SPIR-V straight from LLVM IR.

There may need to be some additional work to map these types to appropriate MVT types in the codegen phases, but my knowledge of codegen infrastructure is somewhat shaky, and this is highly dependent on how different backends wish to translate these types.

hi, @jcranmer-intel , could you please propose a backend demo revision to demonstrate how to use targetext type of backend?

In D139996#4000175, @jrtc27 wrote:

(If the register is marked reserved with -ffixed-xN then you won't get the save/restore code here)

Ping.. Do we still prefer to specify target triple explicitly to make a check?

In D116735#3429850, @craig.topper wrote:

I'm seeing a regression on 401.bzip2 and possibly 471.astar. And I'm not seeing large improvements on 471.omnetpp or 483.xalancbmk.

It's fine for CSKY to use config file. I only have 2 points.

In D121445#3824657, @MaskRay wrote:

mips computed sysroot from GCCInstallation very early in 2013 rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 and Android followed up in 2018 (D45291), but I am not sure this is the correct direction.
They are to support very special distributions. Hard coding the file hierarchy for every Linux distribution just does not scale.
I raised my concern on https://reviews.llvm.org/D134454#3824630 as well.

With https://reviews.llvm.org/D134337 (default configuration file) we should move the logic to use a default configuration file instead.

AFAIK, --target is clang-specific and transparent for compiler user in RV side. -m is also undefined or less used by RV user. It just uses -march to specify 32 or 64 mode and extensions.
Is the convention specified in RV Spec?

In D132205#3780383, @andreadb wrote:

The chage looks good. However, the test must be improved.

Your test only checks the content of the write latency table. However, that is not enough to prove that this bug is fixed; you also need to show that the read advance entry for Read_D is associated with Write_B.

I suggest to modify the InstRW for Inst_C as follows:

def : InstRW<[Write_C, Read_D], (instrs Inst_C)>;

That way, Read_D will be associated to the first read operand of Inst_C.

Since Read_D is now explicitly used, a new MCReadAvanceEntry is added to table MyTargetReadAdvanceTable (see entry #1 below).

// {UseIdx, WriteResourceID, Cycles}
extern const llvm::MCReadAdvanceEntry MyTargetReadAdvanceTable[] = {
  {0,  0,  0}, // Invalid
  {0,  2,  1} // #1
}; // MyTargetReadAdvanceTable

In your test, you need to CHECK the presence of that {0, 2, 1} entry. That's the only way to know that there is a (at least one) SchedRead associated to Write_B (WriteResourceID = 2).
You should also check that Inst_C now uses that ReadAdvance. That's how we know that Read_D is effectively associated with Write_B.

In the .inc output, you should be able to check the last line of the table below:

// {Name, NumMicroOps, BeginGroup, EndGroup, RetireOOO, WriteProcResIdx,#, WriteLatencyIdx,#, ReadAdvanceIdx,#}
static const llvm::MCSchedClassDesc SchedModel_ASchedClasses[] = {
  {DBGFIELD("InvalidSchedClass")  8191, false, false, false, 0, 0,  0, 0,  0, 0},
  {DBGFIELD("Inst_A")             1, false, false, false,  0, 0,  1, 1,  0, 0}, // #1
  {DBGFIELD("Inst_B")             1, false, false, false,  0, 0,  1, 1,  0, 0}, // #2
  {DBGFIELD("Inst_C")             1, false, false, false,  0, 0,  1, 1,  1, 1}, // #3
}; // SchedModel_ASchedClasses

Based on that last descriptor, Inst_C declares a single read-advance entry (i.e. entry #1 in MyTargetReadAdvanceTable).
Entry #1 in MyTargetReadAdvanceTable declares a 1cy of read advance when associated with Write_B.

In conclusion, for completeness, you need to check also tables MyTargetReadAdvanceTable and SchedModel_ASchedClasses.

I hope it helps.

In D132205#3769775, @RKSimon wrote:

Does this fix PR57548?

ping...

Add testcase.

In D129824#3706654, @asb wrote:

@zixuan-wu we discussed this a bit in the last community sync call. One aspect that makes this patch a little hard to review is the lack of a clear patch description summarising the behaviour before the patch, the behaviour after the patch, and the motivation for the change. I appreciate that the time isn't very convenient in all timezones, but if you were able to attend the next call (18th August, same time) to discuss, that might really help agree a path forwards on this change.

In D129824#3699911, @kito-cheng wrote:

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.

---

As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

Is this going to continue?
And also if we use libunwind to handle EH, then it uses dladdr function which need link libdl.

In D129824#3670586, @reames wrote:

This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda when we talk again in two weeks.

Remove unnecessary include header.

In D129824#3653895, @jrtc27 wrote:

Does GCC allow this or not? Because this strikes me as a bad idea at first sight…

In D54214#1737255, @simoncook wrote:

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, rather than using getRISCVArch. I think in the case of -target risc32-..... -mabi=lp64 I think it would confuse users if the tools suddenly changed to doing an rv64 compile. If we disable that, all that function would provide me is the same StringRef I'm already evaluating. I think adding any extra flag to indicate whether a rv32<->rv64 switch is acceptable would just make the code unnecessarily more messy. I think in the future if getRISCVArch evaluates more flags, then it might make sense to reconsider this.

The logic of getIntMatCost and getInstSeqCost is correct to be called in different code place. But the logic is not clear.
So make it clear without still constructing 2 different version of getIntMatCost to reuse the for loop to calculate cost of each chunk.

I found it's no change in .ll test cases. I am wondering if there should be a cost model test case like llvm/test/Analysis/CostModel/ARM/immediates.ll, or make it as NFC patch.

Address comments.

I am curiosity about why Machine Scheduler is better than the List Scheduler after RA?
As I see substitutePass is already used in 3 targets( AArch64,PPC,AMD), should PostMachineSchedulerID going to be the default PostRA Scheduler? @nemanjai @echristo

In D129072#3629019, @liaolucy wrote:

I would like to ask a question, is this patch to reduce code size?

Gentle pin...

In D98002#2657460, @evandro wrote:

In D98002#2657389, @NickHung wrote:

Adding those "sched" information into base RVV instructions seems redundant? The scheduler works on PseudoRVV instructions.

Not redundant, as some tools do use the information at this level, e.g., llvm-mca.

Moreover, this patch captures the resources in a way that it makes it easier to add latency information to specific targets later.

SchedReadWrite needs to consider pseudo instructions with LMUL? Their latency should be different.

And SEW. I'm working on this feature, of which this patch introduces the preliminary information.

BTW, I have run llvm-test-suite, it passed.

In D126451#3539656, @rengolin wrote:

This looks good to me, but wait to make sure others see it, too.

My reasons are: it is largely similar to RISCV implementation, it seems to follow what I expected of the ABI (which is similar to other targets) and has a large corpus of tests.

I can't comment on the specifics of the ABI implementation (I haven't read the ABI document *that* thoroughly), but as David said, issues will be picked up by tests until the target reaches maturity.