Page MenuHomePhabricator

Add MM register mapping from CodeView to MC register id
ClosedPublic

Authored by LuoYuanke on Apr 8 2019, 7:06 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

LuoYuanke created this revision.Apr 8 2019, 7:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 7:06 PM
hans added a reviewer: rnk.Apr 10 2019, 5:51 AM

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

hans added a comment.Apr 11 2019, 2:24 AM

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

Thank you for the suggestion. I disable the code of xmm0 - xmm7 mapping and there is no case failure in llvm/tests/DebugInfo/COFF/. However when I disable general register (rax -r15), some of the test cases (.e.g test/DebugInfo/COFF/types-basic.ll) fail. There is pretty much code for the failed test case which I'm investigating. To create a small test case, we may need to cause much pressure on register allocator for mm0 - mm7 with __m64 variable in c code. However I have not figured out how to created such case.
Nevertheless, does anybody know why the mm0-mm7 registers are not mapped. Is there any reasons on it?

hans added a comment.Apr 11 2019, 6:39 AM

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

Thank you for the suggestion. I disable the code of xmm0 - xmm7 mapping and there is no case failure in llvm/tests/DebugInfo/COFF/. However when I disable general register (rax -r15), some of the test cases (.e.g test/DebugInfo/COFF/types-basic.ll) fail. There is pretty much code for the failed test case which I'm investigating. To create a small test case, we may need to cause much pressure on register allocator for mm0 - mm7 with __m64 variable in c code. However I have not figured out how to created such case.
Nevertheless, does anybody know why the mm0-mm7 registers are not mapped. Is there any reasons on it?

I'm guessing it's just an oversight.

Would it be possible to use inline assembly to trigger use of these registers in some way?

If this turns out to be hard, I think we can just check in your patch as is, it seems like an obvious fix.

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

Thank you for the suggestion. I disable the code of xmm0 - xmm7 mapping and there is no case failure in llvm/tests/DebugInfo/COFF/. However when I disable general register (rax -r15), some of the test cases (.e.g test/DebugInfo/COFF/types-basic.ll) fail. There is pretty much code for the failed test case which I'm investigating. To create a small test case, we may need to cause much pressure on register allocator for mm0 - mm7 with __m64 variable in c code. However I have not figured out how to created such case.
Nevertheless, does anybody know why the mm0-mm7 registers are not mapped. Is there any reasons on it?

I'm guessing it's just an oversight.

Would it be possible to use inline assembly to trigger use of these registers in some way?

If this turns out to be hard, I think we can just check in your patch as is, it seems like an obvious fix.

I'm not familiar with the CodeView stuff, rnk is the better person for that, but would it be possible to add a test that exercises this mapping? Or to put it another way, what does this fix that's currently broken, and is it possible to add a test for it?

I try to add such test case, but I don't find any existing test case for mapping codeveiw register to MC register.
@rnk
Do you know if there is any reference test case in llvm/test code?

You can try removing a bunch of the entries in RegMap and see which tests fail. They're all in llvm/tests/DebugInfo/COFF/
I don't think there's any test for the mapping itself, but the tests rely on it to work.

That's what my second question was about: what is currently not working that your patch is fixing, and can that be used to write a test?

Thank you for the suggestion. I disable the code of xmm0 - xmm7 mapping and there is no case failure in llvm/tests/DebugInfo/COFF/. However when I disable general register (rax -r15), some of the test cases (.e.g test/DebugInfo/COFF/types-basic.ll) fail. There is pretty much code for the failed test case which I'm investigating. To create a small test case, we may need to cause much pressure on register allocator for mm0 - mm7 with __m64 variable in c code. However I have not figured out how to created such case.
Nevertheless, does anybody know why the mm0-mm7 registers are not mapped. Is there any reasons on it?

I'm guessing it's just an oversight.

Would it be possible to use inline assembly to trigger use of these registers in some way?

If this turns out to be hard, I think we can just check in your patch as is, it seems like an obvious fix.

I try to write the test case as below. But it seems mmx register can not be specified explicitly in the clobber list or input list. If I specify "y" constrain, then compiler always allocate mm0 and mm1 register. If I specify the mmx register in the assembly code, then compiler is not aware of it on register allocation.

void test_mmx() {
  __m64 t1, t2, t3, t4;
  //asm("paddsw %0, %1"::"mm1"(t1), "mm2"(t2):);
  asm("paddsw %%mm0, %%mm1":::);
  asm("paddsw %%mm1, %%mm2":::);
  asm("paddsw %%mm2, %%mm3":::);
  asm("paddsw %%mm3, %%mm4":::);
  asm("paddsw %%mm4, %%mm5":::);
  asm("paddsw %%mm5, %%mm6":::);
  asm("paddsw %0, %1"::"y"(t3), "y"(t4):);
  asm("paddsw %0, %1"::"y"(t1), "y"(t2):);
  asm("paddsw %0, %1"::"y"(t1), "y"(t3):);
  __m64 t5 = _m_paddsw(t1, t2);
  __m64 t6 = _m_psubusb(t3, t4);
  __m64 t7 = _m_paddsw(t5, t6);
}
hans accepted this revision.Apr 11 2019, 7:41 AM

Okay, let's not worry too much about the test case, the patch seems obviously good.

Do you have commit access, or would you like me to commit for you?

This revision is now accepted and ready to land.Apr 11 2019, 7:41 AM
This revision was automatically updated to reflect the committed changes.

Okay, let's not worry too much about the test case, the patch seems obviously good.

Do you have commit access, or would you like me to commit for you?

Thank you! I just get the commit permission this week. :) I have committed the patch to llvm repo.