This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Don't use a zero fill with a second parameter
ClosedPublic

Authored by daltenty on Jan 28 2020, 7:21 AM.

Details

Summary

The AIX assembler .space directive can't take a second non-zero argument to fill
with. But LLVM emitFill currently assumes it can. We add a flag to the AsmInfo
to check if non-zero fill is supported, and if we can't zerofill non-zero values
we just splat the .byte directives.

Diff Detail

Event Timeline

daltenty created this revision.Jan 28 2020, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 7:21 AM
daltenty updated this revision to Diff 240879.Jan 28 2020, 7:24 AM
  • Remove include addition
Harbormaster completed remote builds in B45133: Diff 240879.
Xiangling_L added inline comments.
llvm/include/llvm/MC/MCAsmInfo.h
551

Just a minor comment, is it better to name this function ifZeroDirectiveSupportsNonZeroValue() or doesZeroDirectiveSupportsNonZeroValue() since the return value is a boolean?

llvm/lib/MC/MCAsmStreamer.cpp
1114

Is .byte a AIX only directive? If yes, maybe we should assert OS as AIX as well?
And also is this assertion also apply to above ZeroDirective?

llvm/test/CodeGen/PowerPC/aix-nonzero-zerofill.ll
7

Since the intention of this patch includes and if we can't zerofill non-zero values we just splat the .byte directives., we may also want to check correct format for this testcase ?

daltenty marked 3 inline comments as done.Jan 28 2020, 9:24 AM
daltenty added inline comments.
llvm/lib/MC/MCAsmStreamer.cpp
1114

I would expect the .byte directive should be generally applicable (it's supported by GAS among others). The assertion is not, we could in theory emit a non-constant expression for the ZeroDirective and let the assembler do the calculation, but we cannot do this if we are just going to write out the bytes.

daltenty updated this revision to Diff 240903.Jan 28 2020, 9:25 AM
  • Update test to add byte splat
  • Rename MAI query to doesZeroDirectiveSupportNonZeroValue()
daltenty updated this revision to Diff 240906.Jan 28 2020, 9:30 AM
  • Remove erroneously emiting fill length
jasonliu added inline comments.Jan 29 2020, 11:55 AM
llvm/include/llvm/MC/MCAsmInfo.h
182

Have we consider rename the ZeroDirective to FillDirective and fix the comments there?
Since this ZeroDirective did not convey what it really means.
And this query is more like "FillDirectiveSupportsNonZeroFill".

llvm/lib/MC/MCAsmStreamer.cpp
1117

Make it a const variable so that we don't need to make a function call every time in the loop?

daltenty marked 2 inline comments as done.Jan 30 2020, 9:00 AM
daltenty added inline comments.
llvm/include/llvm/MC/MCAsmInfo.h
182

That's not really quite right either. Fill directive seems to imply .fill which has different semantics again (and doesn't exist for the AIX assembler). And if a fill directive only supports zero's isn't it really a zero directive? So maybe there should actually be both?

In any case I'm not sure how much we really want to dig into the naming issue for this patch. We'd likely have to make a fairly widespread change.

daltenty updated this revision to Diff 241484.Jan 30 2020, 9:01 AM
  • Use IntNumBytes to get the length of fill
jasonliu added inline comments.Jan 30 2020, 11:05 AM
llvm/include/llvm/MC/MCAsmInfo.h
182

If we don't want to be too pedantic about naming, let's at least fix the comments about ZeroDirective. From the comment, it says it's used to emit zero bytes. That's definitely not the case here.
These two lines needs to be fixed:
This should be set to the directive used to get some number of zero bytes emitted to the current section.
If this is set to null, the Data*bitsDirective's will be used to emit zero bytes.

llvm/lib/MC/MCAsmStreamer.cpp
1101–1102

nit: const?

1116
1117

instead of ".byte" , use MAI->getData8bitsDirective()?

daltenty marked 2 inline comments as done.Jan 31 2020, 2:18 PM
daltenty updated this revision to Diff 241809.Jan 31 2020, 2:18 PM
  • Update comment
  • Change assertion into report fatal error
  • Address comments
jasonliu accepted this revision.Feb 3 2020, 7:18 AM

LGTM.

This revision is now accepted and ready to land.Feb 3 2020, 7:18 AM
This revision was automatically updated to reflect the committed changes.