Page MenuHomePhabricator

[AArch64ConditionOptimizer] Fix missed optimization due to debug insts [10/10]
ClosedPublic

Authored by vsk on Apr 15 2020, 6:35 PM.

Details

Summary

The findSuitableCompare method can fail if debug instructions are
present in the MBB -- fix this by using helpers to skip over debug
insts.

Diff Detail

Event Timeline

vsk created this revision.Apr 15 2020, 6:35 PM
fhahn added a subscriber: fhahn.Apr 16 2020, 3:08 AM
fhahn added inline comments.
llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
160–161

Might be worth rewriting in terms of an iterator range, e.g. something like reverseInstructionsWithoutDebug(I == MBB->begin() ? I : std::prev(I), MBB->begin())

vsk added inline comments.Apr 16 2020, 1:10 PM
llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
160–161

I have a mild preference for the current version as I feel that the ternary adds some complexity. Happy to change this though if anyone has a strong opinion otherwise.

This revision is now accepted and ready to land.Apr 21 2020, 11:27 AM
paquette added inline comments.Apr 21 2020, 11:29 AM
llvm/lib/Target/AArch64/AArch64ConditionOptimizer.cpp
160–161

It would be nice to be clear that we're skipping debug instructions in the loop header, I suppose?

I think in general it would be nice to avoid having to explicitly skip debug instructions within the loop itself.

vsk updated this revision to Diff 259333.Apr 22 2020, 10:46 AM

Use a range iterator, per review feedback.

This revision was automatically updated to reflect the committed changes.
LukeGeeson added a subscriber: LukeGeeson.EditedApr 23 2020, 6:40 AM

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

I cannot build llvm locally using ninja clean && ninja check-all with this patch due to the following error:

AArch64ConditionOptimizer.cpp: In member function 'llvm::MachineInstr* {anonymous}::AArch64ConditionOptimizer::findSuitableCompare(llvm::MachineBasicBlock*) ... no matching function call to 'prev(llvm::MachineInstr&)'

Using the following cmake command in llvm-project/build

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;lld" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=On -DLLVM_TARGETS_TO_BUILD="ARM;X86;AArch64" -DLLVM_ENABLE_SOLVER_Z3=Off -DPARALLEL_LINK_JOBS=2 -DLLVM_USE_LINKER=gold ../llvm

It looks as though harbourmaster is failing as well.
Based on commit hash 3a53806 on master

I have the same problem trying to build llvm locally.

chill added a subscriber: chill.Apr 23 2020, 7:46 AM

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

I suggest renaming one of the Is.

Looks like it's a scope error in gcc <= 5.5.
https://gcc.godbolt.org/z/ECqRJJ

The minimum required version to build LLVM GCC 5.1.
https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

I have GCC 5.4.

vsk added a comment.Apr 23 2020, 9:24 AM

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

In D78265#1999445, @vsk wrote:

Sorry about the breakage, I've renamed the variable in 210616bd38d589020b45f8cbbf9f9ef1296f2729.

@vsk Thanks very much for the fix.