Page MenuHomePhabricator

[PPC] Expose the ZERO register as a constant physical register
Needs ReviewPublic

Authored by Carrot on Aug 8 2022, 2:45 PM.

Details

Summary

The ZERO register should be exposed as a constant physical register through the interface TargetRegisterInfo::isConstantPhysReg.

Diff Detail

Unit TestsFailed

TimeTest
60,150 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,120 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

Event Timeline

Carrot created this revision.Aug 8 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 2:45 PM
Carrot requested review of this revision.Aug 8 2022, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 2:45 PM

While this is certainly correct, I am curious what prompted the change. Did it cause a test case failure somewhere? Did it prevent some optimization? If either of those is the case, I imagine we can produce a test case for this patch.

Carrot added a comment.Aug 9 2022, 4:22 PM

I'm working on https://reviews.llvm.org/D130919#change-i79krRTVIqHY.

Function MachineRegisterInfo::isConstantPhysReg returns true for a physical register if it satisfies

  1. It is a target specific constant register and
  2. It is not modified in the current function.

But it only checks defined register operands. A physical register may also be clobbered by a RegMask(function call). So my patch adds checking agains regmasks.

This patch causes f128-aggregates.ll failed. The problem is in MachineCSE pass, when it checks if the following instruction can be CSEed,

%22:vsrc = LXVD2X $zero8, %13:g8rc :: (load (s128) from constant-pool)

A physical register $zero8 is used, then it checks if $zero8 is constant through MachineRegisterInfo::isConstantPhysReg. Because it is not marked as target specific constant register, it continues to check if it is modified in the function. $zero8 is not used as defined register operand, so the original MachineRegisterInfo::isConstantPhysReg returns true for it. But now we also consider RegMask clobbered register. There is a function call in the test, $zero8 is not marked as callee saved register(this is another problem), so now $zero8 is modified by a RegMask, and MachineRegisterInfo::isConstantPhysReg returns false for $zero8.

This patch can fix the failure.

arichardson requested changes to this revision.Aug 17 2022, 9:51 AM
arichardson added a subscriber: arichardson.

ping

I'd prefer if this wasn't committed and we used my change to auto-generate this method from tablegen (D131962) instead.

This revision now requires changes to proceed.Aug 17 2022, 9:51 AM

ping

I'd prefer if this wasn't committed and we used my change to auto-generate this method from tablegen (D131962) instead.

@arichardson, I like your patch, thanks!

This change is included in D131962, so this revision can be closed.

arsenm resigned from this revision.Sep 15 2022, 8:01 AM
arichardson resigned from this revision.Sep 19 2022, 7:22 AM
This revision now requires review to proceed.Sep 19 2022, 7:22 AM