Page MenuHomePhabricator

[X86] Updated target specific selection dag code to conservatively check for isAtomic in addition to isVolatile
ClosedPublic

Authored by reames on Aug 15 2019, 4:43 PM.

Details

Summary

See D66309 for context.

This is the first sweep of x86 target specific code to add isAtomic bailouts where appropriate. The intention here is to have the switch from AtomicSDNode to LoadSDNode/StoreSDNode be close to NFC; that is, I'm not looking to allow additional optimizations at this time.

There are only a few todos in this patch, because as far as I can tell, most of the transforms apply to vectors only. At the moment, our atomics for vector story is a bit questionable, and that isn't a near term focus for me.

I can't figure out to exercise the anyext cases; any suggestions on how to write tests which exercise that?

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Aug 15 2019, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 4:43 PM

We have the isSimple() wrapper for in LoadInst and StoreInst, could we add that to MemSDNode as well?

bool isSimple() const { return !isAtomic() && !isVolatile(); }
reames added a comment.Tue, Sep 3, 1:38 PM

We have the isSimple() wrapper for in LoadInst and StoreInst, could we add that to MemSDNode as well?

bool isSimple() const { return !isAtomic() && !isVolatile(); }

I thought about it, but it seemed very likely to be confused the isSimple predicate on value types. I'm happy to add it if you want though.

We have the isSimple() wrapper for in LoadInst and StoreInst, could we add that to MemSDNode as well?

bool isSimple() const { return !isAtomic() && !isVolatile(); }

I thought about it, but it seemed very likely to be confused the isSimple predicate on value types. I'm happy to add it if you want though.

I don't think it'd be confusing, but if you are uncertain, another method name would be fine.

reames updated this revision to Diff 219414.Mon, Sep 9, 1:00 PM
reames edited the summary of this revision. (Show Details)

Rebase, address review comment, and correct patch description

Thanks @reames - no more comments from me.

@craig.topper are you OK with the X86InstrInfo.td changes?

craig.topper accepted this revision.Mon, Sep 9, 4:01 PM

LGTM. Will you also be updating nonvolatile_load/nonvolatile_store in include/llvm/Target/TargetSelectionDAG.td. We use those in several places where we change the size of a load.

This revision is now accepted and ready to land.Mon, Sep 9, 4:01 PM

LGTM. Will you also be updating nonvolatile_load/nonvolatile_store in include/llvm/Target/TargetSelectionDAG.td. We use those in several places where we change the size of a load.

This'll be under a separate review, but yes. See D66318. (I'll move your comment there.)

This revision was automatically updated to reflect the committed changes.