Page MenuHomePhabricator

MC: support different sized constants in constant pools
ClosedPublic

Authored by jannau on Jun 24 2014, 2:44 PM.

Details

Summary

The pseudo instruction ldr <reg>, =... supports on AArch64 32- and 64-bit
constants. Add support for 64 bit constants for the pools to support the pseudo instruction fully.

Changes the AArch64 ldr-pseudo tests to use 32-bit registers and adds tests with 64-bit registers.

Diff Detail

Event Timeline

jannau updated this revision to Diff 10805.Jun 24 2014, 2:44 PM
jannau retitled this revision from to MC: support different sized constants in constant pools.
jannau updated this object.
jannau edited the test plan for this revision. (Show Details)
jannau added a project: deleted.
jannau added a subscriber: Unknown Object (MLST).
jannau updated this revision to Diff 11364.Jul 14 2014, 12:58 AM

adds error when a constant expression is too large for an 32-bit register

jannau updated this object.Jul 14 2014, 1:03 AM
dpeixott edited edge metadata.Jul 14 2014, 11:32 AM

Thanks for the patch. Overall I like the direction you are going. Please see specific comments below.

include/llvm/MC/ConstantPools.h
29

This type is getting a little hard to read. I suggest creating a small struct to represent the constant pool entries and name each field with something meaningful (e.g. Expr, Size, etc).

41

I think we should not use default parameters here so that it is clear that in the API that size is important.

75

Ditto for this default. We can modify the ARMTargetStreamer to always pass 4 to this call since it only ever emits 4-byte constants.

lib/MC/ConstantPools.cpp
34

I think you can simplify the code here by just always emitting the alignment before each entry (EmitCodeAlignment(Size)). You may end up with some redundant alignment directives in the .s file, but it should not cause any code bloat in the final binary because the assembler already keeps track of the alignment as it goes.

This will simplify the logic and remove the need to keep track of MaxAlignment.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3114

Why int64_t here instead of uint64_t? It looks like the code above uses unsigned. Consider using the APInt to make it clear what condition you are checking (e.g. something like APint(Simm, 64).countActiveBits() >= 33)

jannau updated this revision to Diff 11424.Jul 15 2014, 2:34 AM
jannau edited edge metadata.

all review comments addressed.

APInt makes the range check for negative int32_t to uint32_t clearer.

Thanks, this is looking much better. I had one minor suggestion and a question about the APInt usage.

Also, for future patches it would be best to include more context in the patch as described here: http://llvm.org/docs/Phabricator.html. It makes it easier to review.

Thanks for all your work.

include/llvm/MC/ConstantPools.h
44

Add comment for Size param (size in bytes)

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3120

This test still looks a bit hard to understand for me. Are you just checking if the value fits into 32 bits? If so I think APInt::isIntN(32) would be more readable.

jannau updated this revision to Diff 11602.Jul 17 2014, 1:07 PM

updated patch adding the \param comment

dpeixott accepted this revision.Jul 17 2014, 1:59 PM
dpeixott edited edge metadata.

For completeness I would add one more test for loading a smaller constant to an x register to show that we generate a 64-bit entry in the constant pool even if the value does not require the full 64-bits. You don't need another review for that change.

Thanks for all the good work.

This revision is now accepted and ready to land.Jul 17 2014, 1:59 PM
jannau updated this revision to Diff 11617.Jul 17 2014, 3:50 PM
jannau edited edge metadata.

new patch with added since someone needs to commit it for me

added tests for:

  • constant pools, x register and small constants,
  • x register and the constant expressions which are rejected for w registers
  • test to make sure that the size is considered if the todo of merging the same constant to single pool entry is implemented
weimingz accepted this revision.Jul 17 2014, 5:42 PM
weimingz added a reviewer: weimingz.

This patch looks good to me. Thanks for the fix

dpeixott closed this revision.Jul 18 2014, 9:13 AM
dpeixott updated this revision to Diff 11643.

Closed by commit rL213387 (authored by @dpeixott).