Page MenuHomePhabricator

[X86] Alternate implementation of D88194.
ClosedPublic

Authored by craig.topper on Oct 9 2020, 9:18 PM.

Details

Summary

This uses PreprocessISelDAG to replace the constant before
instruction selection instead of matching opcodes after.

Diff Detail

Event Timeline

craig.topper created this revision.Oct 9 2020, 9:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 9:18 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Oct 9 2020, 9:18 PM
xiangzhangllvm added inline comments.Oct 9 2020, 9:59 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
858

Can we replace a constant with a reg without checking the operation?
I am afraid it may cause ISEL failed.

This patch looks more common than D88194. Is it possible that generate endbr constant after PreprocessISelDAG()?

If this is sufficient, I definitely prefer this implementation - much smaller and easier to understand.

craig.topper added inline comments.Oct 10 2020, 7:43 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
858

Cases that require constants should be using ISD::TargetConstant instead of ISD::Constant.

I fixed one case I found where it would break in 375849518db97096212f7f2b996b0d15f97be959. There might be others. I'll try to see if I can find a way to check the isel tables for other cases.

No objections from me and this looks better than D88194

@craig.topper Is there a better way that we can always check for intrinsics that should be using TargetConstant ?

Are we ok with this implementation?

I don't have a great way to protect intrinsics not using TargetConstant properly in the future, but I think we're ok at the moment based on what I see in the isel table.

Are we ok with this implementation?

I don't have a great way to protect intrinsics not using TargetConstant properly in the future, but I think we're ok at the moment based on what I see in the isel table.

LGTM, if problem in the future, I'll fix it. Tkx !!

pengfei accepted this revision.Oct 25 2020, 7:26 PM

LGTM too. I think we can let it in. Thanks.

This revision is now accepted and ready to land.Oct 25 2020, 7:26 PM
This revision was landed with ongoing or failed builds.Oct 27 2020, 12:23 AM
This revision was automatically updated to reflect the committed changes.