This is an archive of the discontinued LLVM Phabricator instance.

Add NoMerge MIFlag to avoid MIR branch folding
ClosedPublic

Authored by zequanwu on May 6 2020, 6:24 PM.

Diff Detail

Event Timeline

zequanwu created this revision.May 6 2020, 6:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 6:24 PM
zequanwu updated this revision to Diff 262523.May 6 2020, 6:56 PM
zequanwu added a reviewer: rnk.

add testcase

zequanwu updated this revision to Diff 263312.May 11 2020, 5:15 PM

Handle nomerge flag in LowerCall at targets AArch64, ARM, PowerPC, RISCV, and add test cases for each target.

rnk edited reviewers, added: craig.topper; removed: rnk.May 13 2020, 12:34 PM
rnk added a subscriber: rnk.May 13 2020, 12:37 PM

Overall, this looks good to me, but I'd like to ask @craig.topper for an opinion. His area of expertise is the x86 backend.

llvm/include/llvm/CodeGen/SelectionDAG.h
281

Please zero initialize this, otherwise the default constructor will not initialize it, and if entries in this hash table are added for other reasons, it will be uninitialized. You could observe this by combining the nomerge attribute with __declspec(allocator), but IMO a test is not necessary.

1916

If NoMerge is false, you can skip this hash table insertion. Otherwise, we will do this hash insertion for each and ever call instruction, but we only need to do it for this feature.

zequanwu updated this revision to Diff 263842.May 13 2020, 1:28 PM
zequanwu marked 3 inline comments as done.

Zero Initialize NoMerge in CallSiteDbgInfo.
Put Check in addNoMergeSiteInfo to avoid unnecessary hash table insertion.

llvm/include/llvm/CodeGen/SelectionDAG.h
1916

Previously, I put the check before each call site of this function.
I agree that put the check inside the function is better.

rnk added a subscriber: vitalybuka.May 13 2020, 1:31 PM

After this lands, can you follow this up by migrating the sanitizer passes to using the attribute instead of using empty inline asm blobs? Send the patch to @vitalybuka. This is the code I'm thinking of:
https://github.com/llvm/llvm-project/blob/028bfdd8913616f7a3e57e8ef5c2a9990e528ff0/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1626
https://github.com/llvm/llvm-project/blob/d6695e18763a05b30cb336c18157175277da8f4b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L864
... and any similar uses.

Run check-asan, check-msan, etc to run the sanitizer integration tests, and confirm that they pass. You can run check-all to run everything and be sure as well.

arsenm added a subscriber: arsenm.May 13 2020, 1:42 PM
arsenm added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
109

It's not obvious to me what this means. These are also supposed to be droppable optimization flags, but it seems like you're using this for something semantically required?

rnk added inline comments.May 13 2020, 1:54 PM
llvm/include/llvm/CodeGen/MachineInstr.h
109

I don't think BundledPred or FrameSetup are droppable, IMO they are semantically significant.

The only alternative I see to this would be to encode this information in the MCInstrDesc, which would require mirroring the entire family of X86 CALL instructions, of which there are many.

In D79537#2035041, @rnk wrote:

After this lands, can you follow this up by migrating the sanitizer passes to using the attribute instead of using empty inline asm blobs? Send the patch to @vitalybuka. This is the code I'm thinking of:
https://github.com/llvm/llvm-project/blob/028bfdd8913616f7a3e57e8ef5c2a9990e528ff0/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1626
https://github.com/llvm/llvm-project/blob/d6695e18763a05b30cb336c18157175277da8f4b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L864
... and any similar uses.

Run check-asan, check-msan, etc to run the sanitizer integration tests, and confirm that they pass. You can run check-all to run everything and be sure as well.

Sure, I will do after this lands.

llvm/include/llvm/CodeGen/MachineInstr.h
109

This NoMerge flag is used to disable branch-fold optimization when present. What do you mean something semantically required?

arsenm added inline comments.May 13 2020, 2:21 PM
llvm/include/llvm/CodeGen/MachineInstr.h
109

I forgot those were also here. Maybe this should be sorted up to the other mandatory flags. I'm pretty sure FrameSetup only changes debug info, so I guess that's not exactly mandatory?

I would still like a better comment here. This one just restates the name of the flag and doesn't tell me what it means by merge

rnk added a comment.May 18 2020, 3:53 PM

I think this looks good, but I want to find another reviewer for CodeGen. @craig.topper or @arsenm, are you OK reviewing this, or can you recommend another reviewer?

llvm/include/llvm/CodeGen/MachineInstr.h
109

I second Matt's comments on reordering the flags to group this flag away from the math flags, which are generally ignorable, and you can expand on this comment here. The notion of "merging" instructions isn't self-explanatory. Perhaps "Passes that drop source location (e.g. tail merging) info should skip this instruction." could work.

zequanwu updated this revision to Diff 264756.May 18 2020, 4:51 PM

Update comment for NoMerge flag

MaskRay added inline comments.May 18 2020, 5:08 PM
llvm/test/CodeGen/PowerPC/nomerge.ll
32

Change the three CHECK: bl bar to CHECK-NEXT: bl bar

It makes the reader clear they are first instructions in their basic blocks and there will be a friendly error message if things go wrong.

; CHECK:      # %if.then2
; CHECK-NEXT:  bl bar
llvm/test/CodeGen/RISCV/nomerge.ll
32

ditto

zequanwu updated this revision to Diff 264759.May 18 2020, 5:40 PM

Change some CHECK to CHECK-NEXT where the call to bar is first instruction in blocks.

zequanwu marked 2 inline comments as done.May 18 2020, 5:41 PM
craig.topper added inline comments.May 27 2020, 11:52 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
4272

Does this need to be in this else or can it apply to both paths?

zequanwu updated this revision to Diff 266648.May 27 2020, 1:34 PM
zequanwu marked an inline comment as done.

rebase and address comment.

llvm/lib/Target/X86/X86ISelLowering.cpp
4272

Yes, it can be applied to both paths.

This revision is now accepted and ready to land.May 29 2020, 11:08 AM
This revision was automatically updated to reflect the committed changes.