The codegen for Windows and Unix is different and 2 of the threads.ll were failing on Windows.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Added a relevant code owner as reviewer. This test has been failing for several weeks now.
The specified XCore backend code owner has moved on from XMOS and from working on XCore. I will have a look at this patch.
Thanks! Note that they are still listed as a code owner - https://github.com/llvm/llvm-project/blob/main/llvm/CODE_OWNERS.TXT#L155.
I've reproduced the failure. The test passes when I change -march=xcore to -mtriple=xcore-unknown-unknown, as in main branch commit https://github.com/llvm/llvm-project/commit/15ca54525d6c2927b2a51b871a9e343c7ce1c2ea. Does that commit solve this problem and unblock D91556 ? (I will continue to investigate to understand the reason for the register swap.)
I think this patch can be cancelled, because the issue is already fixed in the main branch. But please say if I have missed something.
Test threads.ll started failing on Windows in commit https://github.com/llvm/llvm-project/commit/961f31d8ad14c66829991522d73e14b5a96ff6d4 . That commit changed function TargetMachine::shouldAssumeDSOLocal(). Previously, for both Windows and non-Windows, in f_tle in this test, in the DAG combine phase, for global tle:
TargetMachine::shouldAssumeDSOLocal returned true to:
TargetLowering::isOffsetFoldingLegal which returned true to:
SelectionDAG::isConstantIntBuildVectorOrConstantInt which returned global address node to:
DAGCombiner::visitADDLike which put the operands in a certain order.
With the new code, for non-Windows, in f_tle, shouldAssumeDSOLocal returns false, and the operands are not reordered in that way. But for Windows, the function still returns true before the new code is reached, because of special case:
if (TT.isOSBinFormatCOFF() || TT.isOSWindows()) return true;
So the new code produces different output for Windows and non-Windows.
Since we do not need to target Windows on XCore, we do not need to check for specific Windows results, and can use -mtriple to set OS unknown. So I think the existing solution in branch main already resolves the issue of the difference on Windows which this patch is addressing.
That makes sense. If it works and there is no need to verify the tests passes for Windows (with Windows configuration) what we have in master is fine.