This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [TEST] Fix the threads.ll for Windows
AbandonedPublic

Authored by llitchev on Dec 21 2020, 3:18 AM.

Details

Summary

The codegen for Windows and Unix is different and 2 of the threads.ll were failing on Windows.

Diff Detail

Event Timeline

llitchev requested review of this revision.Dec 21 2020, 3:18 AM
llitchev created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 3:18 AM

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.

ftynse added a comment.Jan 5 2021, 2:39 AM

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.

llitchev abandoned this revision.Jan 11 2021, 12:44 PM

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.