Currently the instruction paddi always takes s34imm as the type for the
34 bit immediate. However, the PC Relative form of the instruction should
not produce the same fixup as the non PC Relative form.
This patch splits the s34imm type into s34imm and s34imm_pcrel so that two
different fixups can be emitted.
Details
Details
- Reviewers
nemanjai sfertile lei NeHuang hfinkel kamaub - Group Reviewers
Restricted Project - Commits
- rG97470897c436: [PowerPC] Split s34imm into two types
rGbd2068031121: [PowerPC] Split s34imm into two types
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 460 ms | linux > SanitizerCommon-asan-x86_64-Linux.Linux::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=address -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
| |
| 270 ms | linux > SanitizerCommon-lsan-x86_64-Linux.Linux::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
| |
| 360 ms | linux > SanitizerCommon-msan-x86_64-Linux.Linux::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=memory -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
| |
| 440 ms | linux > SanitizerCommon-tsan-x86_64-Linux.Linux::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
| |
| 300 ms | linux > SanitizerCommon-ubsan-x86_64-Linux.Linux::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=undefined -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/ubsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/ubsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
|
Event Timeline
Comment Actions
This is LGTM, one suggestion on the clang-format change that can be done pre-commit.
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp | ||
|---|---|---|
| 108 | I think it might be a good idea to ignore the clang-format suggestions in this case since the previous way is alot more readable. | |
Comment Actions
Minor nits, otherwise LGTM.
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp | ||
|---|---|---|
| 108 | Yes. Please do not change the existing ones. Unrelated whitespace changes are quite detrimental to meaningful git log history. | |
| llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp | ||
| 413 | I think we can just skip the switch that does nothing and add an llvm_unreachable for this case. | |
| llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
| 439 | It seems very odd to me that we would use the _pcrel version here. There should be no way to do anything PC-relative with this instruction since it will necessarily set the PC-Rel bit to zero. The immediate should always be a real immediate (never any fixup). So although it doesn't matter, we should probably not use the _pcrel version because it will be confusing. I was certainly confused and wrote about 3 versions of this comment :) | |
Comment Actions
Fixed the alignment that was auto-fixed.
Removed the switch that was not needed.
Fixed the types in the TD file to be s34imm.
| llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
|---|---|---|
| 439 | You are correct. | |
Comment Actions
Going to reopen this review.
The initial commit was pulled due to asserts being hit on an release+asserts build.