This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix L[D|W]ARX Implementation
ClosedPublic

Authored by Conanap on Jul 9 2021, 6:08 PM.

Details

Reviewers
nemanjai
Group Reviewers
Restricted Project
Commits
rGf1aca5ac96eb: [PowerPC] Fix L[D|W]ARX Implementation
Summary

LDARX and LWARX sometimes gets optimized out by the compiler
when it is critical to the correctness of the code. This inline asm generation
ensures that it preserved.

Diff Detail

Event Timeline

Conanap created this revision.Jul 9 2021, 6:08 PM
Conanap requested review of this revision.Jul 9 2021, 6:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 9 2021, 6:08 PM
Conanap added reviewers: Restricted Project, nemanjai.Jul 9 2021, 6:09 PM
Conanap updated this revision to Diff 357680.Jul 9 2021, 6:23 PM

Update test cases

Conanap updated this revision to Diff 357681.Jul 9 2021, 6:36 PM

Added modifier to $1

Conanap updated this revision to Diff 357684.Jul 9 2021, 6:57 PM

Fixed more test cases

lkail added a subscriber: lkail.Jul 9 2021, 6:57 PM
lkail added inline comments.
llvm/include/llvm/IR/IntrinsicsPowerPC.td
1571

Just curious, compiler optimizes the instruction out even setting IntrArgMemOnly and IntrReadMem here?

nemanjai added inline comments.Jul 9 2021, 7:23 PM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
1571

Yes. Well, those two more or less tell it to go ahead and optimize it out if there are no uses. What is more surprising is that even with IntrHasSideEffects. Heck, it even optimizes with all of:

IntrReadMem
IntrArgMemOnly
IntrHasSideEffects
Throws
IntrNoDuplicate
IntrNoMerge

I certainly found that surprising but I assume there is some justification as to why that is. There is some precedent for producing inline asm for builtins (see EmitX86BitTestIntrinsic() and similar). I don't know why that intrinsic was implemented using inline asm (commit messages don't say) but I presume it might have been for a similar reason.

looking at the failing test cases, for example ./clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vadd.c, which are not test cases compiled for PPC, I'm seeing this following error:

clang: /home/conanap/llvm/ccom/llvm-project/llvm/lib/IR/Instructions.cpp:494: void llvm::CallInst::init(llvm::FunctionType *, llvm::Value *, ArrayRef<llvm::Value *>, ArrayRef<llvm::OperandBundleDef>, const llvm::Twine &): Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && "Calling a function with a bad signature!"' failed.

Debug prints show that it's entering the code block emitLoadReserveIntrinsic and BuiltinID evaluates to clang::PPC::BI__builtin_ppc_lwarx. The FTy dump shows i32 (i32 *), which is correct... I'm not exactly sure what to make of this yet, just putting this update here.

lkail added inline comments.Jul 10 2021, 9:41 PM
clang/lib/CodeGen/CGBuiltin.cpp
997

Maybe rename to emitPPCLoadReserveIntrinsic should be more appropriate, since other targets also have similar load-reserve/load-linked/load-acquire notions and what this function does is ad hoc to PPC.

lkail added inline comments.Jul 10 2021, 9:46 PM
clang/lib/CodeGen/CGBuiltin.cpp
1015

Adding default: llvm_unreachable would be nice.

nemanjai requested changes to this revision.Jul 11 2021, 4:08 PM

I believe that your failing test case is because you are attempting to emit code for these builtins in the target independent code and the values just happen to match up.

clang/lib/CodeGen/CGBuiltin.cpp
997

+1

1000

Nit: please follow naming conventions. Variables are to be capitalized.

1015

+1

5146

Why?
Everything else is Builtin::BI__<builtin_name> but these are
clang::PPC::BI__<builtin_name> for some reason.

That doesn't really make sense to me.
Also, what on earth is this doing here? This is a PPC builtin. Those should be handled in EmitPPCBuiltinExpr() and not here.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll
18–19

This is not the asm that the front end generates. Why would you generate one thing in the front end and then test a different thing in the back end?

This revision now requires changes to proceed.Jul 11 2021, 4:08 PM
Conanap updated this revision to Diff 357832.Jul 11 2021, 8:02 PM
Conanap marked 6 inline comments as done.

Moved implementation to a more appropriate function,
updated test cases.

Conanap updated this revision to Diff 357833.Jul 11 2021, 8:04 PM

Removed unintended change

Conanap added inline comments.Jul 12 2021, 11:33 AM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll
18–19

I'm not quite sure what you mean by this; the IR output is taken from the .c test case above.

nemanjai added inline comments.Jul 12 2021, 12:13 PM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll
18–19

I don't think that is the case.
Above:
call i64 asm sideeffect "ldarx $0, ${1:y}", "=r,*Z,~{memory}"(i64* %a)
Here:
call i64 asm sideeffect "ldarx $0, $1", "=r,*Z,~{memory}"(i64* %a)

Conanap added inline comments.Jul 12 2021, 12:23 PM
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll
18–19

ah I see I missed the modifier.

Conanap updated this revision to Diff 358028.Jul 12 2021, 12:27 PM

Updated a test case

Conanap updated this revision to Diff 358032.Jul 12 2021, 12:34 PM

Updated lwarx test case with modifier

Conanap marked 2 inline comments as done.Jul 12 2021, 1:34 PM
nemanjai accepted this revision.Jul 12 2021, 2:27 PM

LGTM.

This revision is now accepted and ready to land.Jul 12 2021, 2:27 PM
This revision was automatically updated to reflect the committed changes.