This is an archive of the discontinued LLVM Phabricator instance.

[X86] Zero some outputs of Keylocker intrinsics in error case
ClosedPublic

Authored by xiangzhangllvm on Jun 23 2021, 1:24 AM.

Details

Summary

Some Keylocker intrinsics will set Zero flag if run error, in this case, we will zero the output of their intrinsic.

Diff Detail

Event Timeline

xiangzhangllvm requested review of this revision.Jun 23 2021, 1:24 AM
xiangzhangllvm created this revision.
xiangzhangllvm added a reviewer: FreddyYe.
xiangzhangllvm added a comment.EditedJun 23 2021, 5:23 PM

"x64 debian build failed" should has no relation with this patch, PLS ignore it.

I checked the log of "x64 debian failed": (And another patch "https://reviews.llvm.org/D104773" also has the same build fail.)

tools/flang/include/flang/Optimizer/Dialect/FIROps.h.inc:1480:32: error: functions that differ only in their return type cannot be overloaded
    static constexpr StringRef calleeAttrName() { return "callee"; }

                     ~~~~~~~~~ ^
tools/flang/include/flang/Optimizer/Dialect/FIROps.h.inc:1452:22: note: previous definition is here
  ::mlir::Identifier calleeAttrName() {

To keep the same style of the old code, PLS ignore the tools clang-tidy and clang format warning.

pengfei added inline comments.Jun 24 2021, 1:32 AM
clang/lib/CodeGen/CGBuiltin.cpp
14744–14764

Do we need to distinguish 128 and 256. Should it better just use "aesenc_error" etc.?

14851–14856

Can we zero the memory in one-off?

clang/test/CodeGen/X86/keylocker.c
2

Can we use O2 to reduce the CHECKs or only check the buildin and branch instrcutions?

xiangzhangllvm added inline comments.Jun 27 2021, 5:07 PM
clang/lib/CodeGen/CGBuiltin.cpp
14744–14764

I didn't distinguish before, to let the symbol be more clear with their intrinsics, I distinguish them.

14851–14856

Sorry, what is the "one-off" means ?

clang/test/CodeGen/X86/keylocker.c
2

clang_cc1 is default to O0, I think there is a beneafit to not bring into optimization changes in the future for CHECKs.

pengfei accepted this revision.Jun 28 2021, 5:42 AM

LGTM.

clang/lib/CodeGen/CGBuiltin.cpp
14851–14856

I mean you can clear the memory in one time, e.g. Builder.CreateMemSet(Ptr, Builder.getInt8(0), 16 * 8, 16)

This revision is now accepted and ready to land.Jun 28 2021, 5:42 AM
This revision was landed with ongoing or failed builds.Jun 28 2021, 10:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 10:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
craig.topper added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14834

Sorry I'm late here. Instead of having 3 separate strings for each intrinsic can you do something like createBasicBlock(BaseName + "_no_error"...) here. I believe createBasicBlock takes a Twine.

xiangzhangllvm added inline comments.Jul 2 2021, 1:02 AM
clang/lib/CodeGen/CGBuiltin.cpp
14834

Hello Craig! Nice to meet you again :) , Sorry for not noticing you this patch before. I planned not to disturb you if not necessary.

Yes, using Twine will more clear, let me refine it. Thanks a lot!