Page MenuHomePhabricator

[PowerPC] intrinsic should not have flag isBarrier.

Authored by shchenz on Dec 9 2018, 5:32 PM.



This is an issue found when testing PowerPC with verify-machineinstrs.

llc -mtriple=powerpc64-unknown-linux-gnu < llvm/llvm/test/CodeGen/PowerPC/sj-ctr-loop.ll -verify-machineinstrs

Bad machine code: MBB exits via unconditional fall-through but ends with a barrier instruction! ***

        function: main
        basic block: %bb.2 (0x100275437e8)

Content in block BB.2:

BB#2: derived from LLVM BB %for.end
Predecessors according to CFG: BB#1
%vreg2<def> = ADDIStocHA %X2, <ga:@env_sigill>; G8RC_and_G8RC_NOX0:%vreg2
%vreg3<def> = LDtocL <ga:@env_sigill>, %vreg2<kill>; mem:LD8[GOT] G8RC:%vreg3 G8RC_and_G8RC_NOX0:%vreg2
%vreg4<def> = EH_SjLj_SetJmp64 %vreg3<kill>, %CTR8<imp-def,dead>; GPRC:%vreg4 G8RC:%vreg3

A barrier instruction means control flow can not fall through like and unconditional jumps. But setjmp should fall through. intrinsic should not have flag isBarrier.

The original case fails at another place after resolving setjmp barrier issue. So I create a new case. And I only focus on PowerPC target, there are some other platforms like ARM, X86 also have same issue.

Diff Detail


Event Timeline

shchenz created this revision.Dec 9 2018, 5:32 PM
shchenz edited the summary of this revision. (Show Details)Dec 9 2018, 5:50 PM
nemanjai accepted this revision.Dec 12 2018, 4:51 AM
nemanjai added a subscriber: craig.topper.

Since we have discovered this in the PPC back end but the issue also exists in ARM and X86, it'd be nice to notify @t.p.northover (already a reviewer) and @craig.topper about this potentially needing to change in their back ends as well.

In any case, other than the minor comments, the PPC portion LGTM.

349 ↗(On Diff #177449)

Please add a comment along the lines of:
While longjmp is a control-flow barrier (fallthrough isn't allowed), setjmp is not.

In both places where we define these (32 and 64 bit).

163 ↗(On Diff #177449)

Put the comment on a separate line. I think this will make FileCheck actually look for all of that text which will clearly always make this test trivially succeed. Also, please correct the typo buildin -> builtin.

This revision is now accepted and ready to land.Dec 12 2018, 4:51 AM
shchenz updated this revision to Diff 178033.Dec 13 2018, 4:00 AM

Update according to Nemanjai's comments

This revision was automatically updated to reflect the committed changes.