Only checks if there are no sub registers involved for now.
There are lots of failures currently which need to be fixed by various backends.
Splitting this off from https://reviews.llvm.org/D37640
Only checks if there are no sub registers involved for now.
There are lots of failures currently which need to be fixed by various backends.
Splitting this off from https://reviews.llvm.org/D37640
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
981 | For X86 back-end this is not always correct. def FR32 : RegisterClass<"X86", [f32], 32, (sequence "XMM%u", 0, 15)>; def VR128 : RegisterClass<"X86", [v4f32, v2f64, v16i8, v8i16, v4i32, v2i64], 128, (add FR32)>; For example the test fail DAG/GISEL (with the patch) define float @test_add_float(float %arg1, float %arg2) { %ret = fadd float %arg1, %arg2 ret float %ret } for the correct MIR. (xmm size is 128) %0(s32) = COPY %xmm0 %1(s32) = COPY %xmm1 %2(s32) = G_FADD %0, %1 %xmm0 = COPY %2(s32) Thanks |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
981 | I'd argue that when generic types are on one side of a copy, i.e., this is a pre-isel copy, we want the size to exactly match. That being said, the check is too broad. We need to check that only for pre-isel copy. Regular copies require indeed target knowledge (e.g., eflags = copy gpr is valid on x86). |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
981 | I can update the check to do it only when there are generic types involved (ie one of src and dst has a generic virtual register). |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
981 | That would be perfect. |
Updated to refactor getRegSizeInBits into TargetRegisterInfo. There are 57 failures in total.
LGTM with one suggestion
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
985 | Seeing this pattern being actually the implementation of RegisterBankInfo::getSizeInBits, I would suggest to move the whole RegisterBankInfo::getSizeInBits directly in TargetRegisterInfo. |
Also, there are various tests that need to be updated by the various backends, and I can only land once those are updated. I haven't had time to update all the backends.
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
985 | I thought TargetRegisterInfo doesn't have LLT headers pulled in (but looks like it is getting transitively pulled in). I can pretty much move the implementation of RegisterBankInfo::getSizeInBits into TargetRegisterInfo. |
Hi, here's the patch for the ARM GlobalISel tests: https://reviews.llvm.org/differential/diff/119563/
Let me know if there's anything else that needs addressing on ARM.
Thanks Diana.
@igorb would it be possible for you to look at X86 when you get a chance? If not, I can update it.
Hi All,
I will try to cover for Igor in helping with the X86 part of this patch.
AFAIU, the challenge is in getting the call/return lowering working for the case where a function's argument/return-value is a f32 or a f64. Some subtargets will use the FP stack registers (which are 80-bit wide), and other will use XMM's (which are 128-bit wide).
Example:
define float @foo(float %arg1, float %arg2) { ret float %arg2 }
Existing state on ToT: llc -mtriple=x86_64-linux-gnu -mattr=+sse2 -global-isel -stop-after=irtranslator
body: | bb.1 (%ir-block.0): liveins: %xmm0, %xmm1 %0:_(s32) = COPY %xmm0 %1:_(s32) = COPY %xmm1 %xmm0 = COPY %1(s32) RET 0, implicit %xmm0
Here are some options for resolving this issue:
I haven't tried implementing any of the above options, and given my limited knowledge in GlobalISel, would appreciate any feedback on this.
Thanks, Zvi
I would definitely prefer the sizes to be right coming out of the CallLowering - so option #1.
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
984 | What if neither of the registers has a class? That would mean, both registers have LLTs. Then, the check won't complain if those LLTs have the same size in bits, but don't exactly match, like v2s32 and v4s16, for instance. Shouldn't LLTs match exactly if both are valid? |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
984 | Good catch. I should be checking LLTs are equal here. I will update the patch. |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
976 | Here we rely on having operands in MI, while previously in MachineVerifier::visitMachineInstrBefore we could discover that there are not enough operands given. Should we guard that with foundErrors? test/CodeGen/MIR/X86/machine-verifier.mir |
lib/CodeGen/MachineVerifier.cpp | ||
---|---|---|
976 | Sounds sensible to me. | |
lib/CodeGen/TargetRegisterInfo.cpp | ||
455 | This function is not consistent with RegisterBankInfo::getSizeInBits in the priority it gives to the different size when several sources are available (RC, Type). RBI goes Type if any then RC, this one does the opposite and I believe this is not what we want. At the very least, RBI should use this implementation instead of duplicating the logic. |
X86 currently had/has three kinds of fails after this patch:
Updated based on Quentin's/Roman's feedback.
With this patch, only X86 backend fails.
I can push the getSizeInbits change in a NFC change all by itself, and then the verifier change can land separately once X86 finishes it?
lib/CodeGen/TargetRegisterInfo.cpp | ||
---|---|---|
455 | Good catch. Will fix this. |