Add support for more archs in `Triple::getArchTypeForLLVMName`

[antoniofrighetto]

Summary

Add support for i386, s390x in Triple::getArchTypeForLLVMName.

[probinson]

Test? I'm a little surprised that changing the spelling of an arch didn't cause any failures.

[antoniofrighetto]

In D122003#3392424, @probinson wrote:

Test? I'm a little surprised that changing the spelling of an arch didn't cause any failures.

To my surprise too, it actually did. I updated the diff by adding x86_64 alongside x86-64.

[probinson]

You still need a test for this change. It seems there isn't really an existing test for this API; it's used in llvm/unittests/MC/TargetRegistry.cpp, but that's testing the registry rather than the API.

[rengolin]

This is already tested by its users (via lookupTarget) in all sorts of places, but not this change in particular, which is the problem.

But I think testing this as an API (by checking that all names resolve to the correct arch) is too trivial to be worth doing it.

So I recommend you submit the actual change you need this for, and only what you need from this patch on it. If the new change has tests (compiling, linking) on the arch you need, than that'll be the test.

It's unclear to me why you have to add x86_64 after adding new cases to the switch, so I suggest you don't add more than what you need on your actual patch.

I think once we know where this is coming from and why you need it, and you have tests for that, we'll be comfortable in knowing this is being tested.

[antoniofrighetto]

I have updated the patch by removing the case for x86_64. Although it looks like that's a known problem in LLVM (converting the arch-name to Triple::x86_64 and then back to string does not return the original name), this may warrant a separate fix; so that, should something break up, we may easily revert it, and not blame this change.

@probinson, considering that getArchTypeForLLVMName does not seem to be tested anywhere, and proposing a new one for this small change may not be worth it, could it make sense not to a provide a test at this time?

[rengolin]

My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.

So this review should be abandoned in favour of the actual s390x review that needs it, with this change in it.

[probinson]

In D122003#3399214, @rengolin wrote:

My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.

So this review should be abandoned in favour of the actual s390x review that needs it, with this change in it.

Works for me.

[antoniofrighetto]

@probinson, sorry for late reply, it turns out that the sub-project, which this change was needed for, has been recently dismissed, so no longer needed. Also, we did not have any change of ours to provide, alongside the patch. That said, considering that the inverse function parseArch already supports s390x and the proposed change is fairly minimal (besides that getArchTypeForLLVMName is already tested via lookupTarget), I'm still wondering if part of the patch (at least the case adding s390x arch) could be beneficial. If you think it's not, I'll close the review.

[rengolin]

Honestly, I don't see why this commit would be a problem without tests, given there aren't tests for the rest. It seems like an obvious omission to me.

[probinson]

OK. It is indeed a small obvious fix.

[rengolin]

Thanks!

llvm/lib/Support/Triple.cpp

return StringSwitch<Triple::ArchType>(Name)
     .Case("r600", r600)
     .Case("amdgcn", amdgcn)
     .Case("riscv32", riscv32)
     .Case("riscv64", riscv64)
     .Case("hexagon", hexagon)
     .Case("sparc", sparc)
     .Case("sparcel", sparcel)
     .Case("sparcv9", sparcv9)
+    .Case("s390x", systemz)
     .Case("systemz", systemz)
     .Case("tce", tce)
     .Case("tcele", tcele)
     .Case("thumb", thumb)
     .Case("thumbeb", thumbeb)
     .Case("x86", x86)
+    .Case("i386", x86)
     .Case("x86-64", x86_64)
     .Case("xcore", xcore)
     .Case("nvptx", nvptx)
     .Case("nvptx64", nvptx64)
     .Case("le32", le32)
     .Case("le64", le64)
     .Case("amdil", amdil)
     .Case("amdil64", amdil64)