Page MenuHomePhabricator

[PowerPC] Only use some extend mne if assembler is modern enough
ClosedPublic

Authored by jsji on Jan 11 2021, 3:10 PM.

Details

Summary

Legacy AIX assembly might not support all extended mnes,
add one feature bit to control the generation in MC,
and avoid generating them by default on AIX.

Diff Detail

Event Timeline

jsji created this revision.Jan 11 2021, 3:10 PM
jsji requested review of this revision.Jan 11 2021, 3:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 3:10 PM
jsji updated this revision to Diff 316119.Jan 12 2021, 9:14 AM

Rebase after landing D94449 to retrigger precommit tests.

clang -target powerpc64le-unknown-linux-gnu  -c -o a.o  modern-as.s
 error: instruction use requires an option to be enabled
            mfudscr 2

IIUC this patch is breaking compatibility with gas due to the AIX assembler not supporting the mnemonic. Similarly for my xxspltd patch, we wouldn't be able to encode the mnemonic without passing extra options on Linux. Thats why I took the approach I did in D94419. I am happy to change direction, but in my opinion we should only be disabling the mnemonics on AIX.

jsji added a comment.Jan 12 2021, 10:37 AM

Yes, this patch should only disable it for AIX.
See the changes in Taget Machine.
I don't know why you get that failure, I will check.

jsji updated this revision to Diff 316215.Jan 12 2021, 1:22 PM

Update feature for MC Layer as well, also update encoding test to test triple.

jsji added a comment.Jan 12 2021, 1:24 PM

@sfertile I have fixed the MC layer feature as well, please have another look. Thanks.

Thanks Jinsong, left a couple really minor comments. Will rebase the xxspltd fix on this patch now.

llvm/lib/Target/PowerPC/PPC.td
60

Minor nit: I think name, option, description string, etc should explicitly reflect its the AIX assembler.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
1122

ditto in adding AIX to the name.

llvm/test/MC/PowerPC/modern-as.s
2 ↗(On Diff #316215)

We should have a run step that checks for the error when '-mattr=-modern-as`.

llvm/test/MC/PowerPC/ppc64-encoding-ext.s
3422 ↗(On Diff #316215)

We can leave this test alone now that the mnemonic is accepted by default.

jsji updated this revision to Diff 316425.Jan 13 2021, 9:12 AM

Address comments.

jsji added inline comments.Jan 13 2021, 9:14 AM
llvm/lib/Target/PowerPC/PPC.td
60

I updated the description and comments to mention AIX, but not the naming of feature.
As I think we should leave this as a general feature that can be used in the future for similar workaround, not necessary AIX only.

sfertile added inline comments.Jan 14 2021, 6:34 AM
llvm/lib/Target/PowerPC/PPC.td
60

If we find a bug in gas that necessitate disabling an alias and we attempt to use this same feature it would mean also disabling xxspltd and mtudscr on Linux breaking compatibility with gas. Likewise we would be disabling said alias on AIX breaking compatibility with the AIX system as for a gas bug.

jsji added inline comments.Jan 14 2021, 7:07 AM
llvm/lib/Target/PowerPC/PPC.td
60

Yes, unless we want to give one feature for each assembler limitation , there will be situation like, compiler will try to be conservative.

sfertile added inline comments.Jan 14 2021, 8:47 AM
llvm/lib/Target/PowerPC/PPC.td
60

If we were only talking about what the compiler emits then I would agree with you 100%. But the Feature also affects what the integrated assembler accepts, in which case breaking compatability due to bugs on another platform isn't acceptable.

There might be another approach though: we don't need to disable accepting the bugged/missing mnemonics in the integrated assembler (ie we always add +modern-as to the MCSubtargetInfo). Then we can have the conservative behaviour in what the compiler emits, still accept anything the reference assembler does, and not have to have multiple Features to control the behaviour. The down side is there is assembly integrated-as will accept that the reference assembler won't.

jsji updated this revision to Diff 316675.Jan 14 2021, 8:58 AM

Rename the feature name to be AIX specific.

jsji added inline comments.Jan 14 2021, 9:00 AM
llvm/lib/Target/PowerPC/PPC.td
60

Anyway, I updated the name to be AIX specific now. We can introduce new feature bits for Linux later if really needed.

jsji updated this revision to Diff 316691.Jan 14 2021, 9:52 AM

Introduce AIX feature bit, and only append this on AIX.

jsji updated this revision to Diff 316700.Jan 14 2021, 10:16 AM

Rebase and update tests.

jsji updated this revision to Diff 316702.Jan 14 2021, 10:20 AM

Update encoding testcases.

Harbormaster completed remote builds in B85198: Diff 316702.
sfertile accepted this revision as: sfertile.Jan 14 2021, 12:13 PM

Thanks Jinsong, LGTM.

This revision is now accepted and ready to land.Jan 14 2021, 12:13 PM