This is an archive of the discontinued LLVM Phabricator instance.

[AArch64InstPrinter] Change printAddSubImm to only add imm value comment when shifted
ClosedPublic

Authored by jasonmolenda on Jul 30 2021, 2:48 PM.

Details

Summary

AArch64InstPrinter::printAddSubImm adds an immediate value to the comment stream when both are available. This is helpful when the imm value is shifted, e.g.

add x9, x0, #291, lsl #12 ; =1191936

but most of the time it looks like

subs x9, x0, #256 ; =256

where the comment adds nothing useful.

This patch changes printAddSubImm so it only appends the immediate value to the comment stream when it has a shift. The majority of the patchset is updating the tests to match the new output style.

Diff Detail

Event Timeline

jasonmolenda created this revision.Jul 30 2021, 2:48 PM
jasonmolenda requested review of this revision.Jul 30 2021, 2:48 PM
MaskRay accepted this revision.Jul 30 2021, 3:38 PM

Thanks for the patch! I have wanted this for some time.

This revision is now accepted and ready to land.Jul 30 2021, 3:38 PM
MaskRay added a comment.EditedJul 30 2021, 3:38 PM

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

An interesting side question as long as we're talking about this printer,

add x9, x0, #291, lsl #12 ; =1191936

once we have a shifted imm value, adding the value in base10 to the comment stream is maybe not the best choice. This could be printed as,

add x9, x0, #291, lsl #12 ; =0x123000

Anyone have an opinion one way or the other?

Currently --print-imm-hex can change ; =1191936 to the hexadecimal form.

I do feel unconditionally hex comments are better and think an option for decimal comment may unlikely be needed.

So LGTM as a follow-up.

llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-align.s
6

Perhaps append {{$}} to ensure there is no comment.

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

I figured I would do commits locally and push them at the same time. I'm doing a final testsuite run right now and was hoping to land it if you think this might be a poor idea.

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

I figured I would do commits locally and push them at the same time. I'm doing a final testsuite run right now and was hoping to land it if you think this might be a poor idea.

I wouldn't land them as two separate commits - even though the build bots might be fine, it'll mean that for at least one commit, LLVM as a whole is in a broken state, which can cause various issues e.g. when bisecting, or if a system does a commit-by-commit build and test cycle.

llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-align.s
6

+1

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

I figured I would do commits locally and push them at the same time. I'm doing a final testsuite run right now and was hoping to land it if you think this might be a poor idea.

I wouldn't land them as two separate commits - even though the build bots might be fine, it'll mean that for at least one commit, LLVM as a whole is in a broken state, which can cause various issues e.g. when bisecting, or if a system does a commit-by-commit build and test cycle.

Ah of course, good point.

I added the {{$}} in my WIP patch.

Consider committing the test update separately from the code patch, so that the functional change has very few lines of update.

Wouldn't that mean a revision with lots of tests failing? I think a single large commit would be better. People can grep out changes in llvm/test if they're not interested in that.

I am referring to // =16 removal. Deleting the part will not make the test fail.