Page MenuHomePhabricator

[ARM] llc -arm-execute-only with floating point runs into UNREACHABLE
ClosedPublic

Authored by labrinea on Jun 1 2017, 5:22 AM.

Details

Summary

The DAG Combiner tries to generate literal pools for floating point constants when both -arm-execute-only and vfp instructions are enabled:
(a cond b) ? 1.0f : 2.0f -> load (tmp + ((a cond b) ? 0 : 4)

reproducer.ll:

define arm_aapcs_vfpcc float @lower_fpconst_select(float %f) {
  %cmp = fcmp nnan oeq float %f, 0.000000e+00
  %sel = select i1 %cmp, float 5.000000e+08, float 5.000000e+09
  ret float %sel
}

$ llc -mtriple=thumbv8m.main -arm-execute-only -mattr=fp-armv8 reproducer.ll
execute-only should not generate constant pools
UNREACHABLE executed at llvm/lib/Target/ARM/ARMISelLowering.cpp:7640!

Diff Detail

Repository
rL LLVM

Event Timeline

labrinea created this revision.Jun 1 2017, 5:22 AM

There are a bunch of places in SelectionDAG which generate constant pools for obscure constructs. Maybe we need a separate TargetLowering hook to suppress constant pools in general, rather than just suppressing this particular DAGCombine? (grep for getConstantPool in lib/CodeGen/SelectionDAG/ .)

Alternatively, how hard would it be to teach the ARM backend to put constant pools into a data section?

labrinea updated this revision to Diff 101550.Jun 6 2017, 6:35 AM

Instead of suppressing this particular reproducer, teach the compiler to lower constant pools as global values (literals in the data section) when generating execute-only code.

Most targets keep the constants in the MachineConstantPool, and emit them with AsmPrinter::EmitConstantPool. I would prefer to align with the way other targets handle constant pools, if it isn't too hard.

christof edited edge metadata.Jun 7 2017, 5:16 AM

The ARM code generator emits constant pools in a custom way. I believe that is to get them in-line with the code as a constant-pool in .text. But I think that Eli Friedman is right that for this use case it is possible to use the standard LLVM way of emitting them.

test/CodeGen/ARM/constantfp.ll
180 ↗(On Diff #100991)

typo: independent

184 ↗(On Diff #101550)

I assume 'tmp' here refers to the address of a constant pool that contains the 2 floating point constants. You could use something like 'ConstantPoolAddr' instead of 'tmp' to make that more explicit.

labrinea updated this revision to Diff 101873.Jun 8 2017, 2:40 AM

When generating execute-only code invoke the generic AsmPrinter in order to place the literals in the data section, otherwise handle Constant Pools in EmitInstruction.

labrinea updated this revision to Diff 101876.Jun 8 2017, 3:01 AM

Oops, I forgot to remove the llvm_unreachable() check from ARMISelLowering.

This might be a bit more difficult then expected. The code generator assumes the constant pool is PC-rel addressable using ADR at the moment. This is not the case when the constant pool lives in a different section. I should have known, sorry.

test/CodeGen/ARM/constantfp.ll
205 ↗(On Diff #101876)

I doubt this is correct. ADR constructs a PC relative address with 8-bit immediate, but the constant pool is in data which might not be in range. I would expect a movw/movt to load the constant pool's address.

labrinea updated this revision to Diff 102005.Jun 9 2017, 3:02 AM

Fixed the way we materialise the address of a constant pool when generating execute-only code. Use movw/movt instead of pc-relative access.

If find my original approach in diff1 less ad-hock than the latest one diff5. Lowering a Constant Pool as a Global Variable at the DAG level seems to me more legitimate than printing movw/movt for a LEApcrel machine instruction and then using the generic asm printer for the constant pool itself.

This patch is definitely not the right approach. The LEA pseudo instruction should never be selected. I had a bit of a closer look and it seems that (ARMWrapper tconstpool) has various patterns that all match to PC relative pseudo instructions. They all should not happen in XO, so maybe this work is a bit more involved then expected. Also, we still have the ConstantIslandPass that is going to assume these constants pools are present in text and will split basic blocks for no good reason.

I have to agree with Alexandros that the earlier approach of custom lowering the constant pool is more in line with the way the ARM Target is structured. @efriedma: any different opinion?

lib/Target/ARM/ARMAsmPrinter.cpp
1250 ↗(On Diff #102005)

These *LEApcrel are pseudo instructions that load a PC-relative address. They should never expand to MOVW/MOVT.

Custom-lowering the constant pool to a global isn't that terrible, I guess... it's ugly, and we can't share the globals across basic blocks, but it should behave correctly, as far as I know.

lib/Target/ARM/ARMAsmPrinter.cpp
1250 ↗(On Diff #102005)

Yes, we shouldn't be messing with ARMAsmPrinter::EmitInstruction.

labrinea updated this revision to Diff 102305.Jun 13 2017, 2:46 AM

It's unfortunate that we cannot share the globals across basic blocks. However, lowering a constant pool as a global variable guarantees that execute-only plays well with all the position-independent addressing modes out of the box. In order to support those with execute-only we would need to add new instruction patterns and logic to handle them in various places, which might introduce new bugs. I've switched back to the previous approach and added a few more checks to verify the CodeGen with position-independent addressing modes. I've also removed few more unreachable checks that can now be reached with PIC. Christof are you happy with it?

The execute-only and PIC features cannot be enabled together. You probably should leave that unreachable in lib/Target/ARM/ARMAsmPrinter.cpp in place. You might even add one under the case ARM::CONSTPOOL_ENTRY in that file for good measure.

Otherwise, I'm happy to approve the constpool to global constant lowering

test/CodeGen/ARM/constantfp.ll
226 ↗(On Diff #102305)
  • Sorry, but this is wrong. The point of execute-only is that there is no data access to any text section.
labrinea updated this revision to Diff 102335.Jun 13 2017, 8:29 AM

Hi Christof,

I wasn't aware that PIC is not compatible with execute-only. Indeed a constant pool in text is being generated if I remove those checks. It contains the distance between the constant pool that is located in the data section and the address of the "current" instruction. I've kept those checks against constant pool generation and added another one in ARMAsmPrinter.cpp. I've also removed the PIC tests since they are now irrelevant.

Cheers,
Alexandros

I was about to approve it but, I have 1 little nitpick that I almost missed. Sorry for the dribble feed. It's about the naming of the global.

lib/Target/ARM/ARMISelLowering.cpp
2690 ↗(On Diff #102335)

I expect you meant to use CP (from ConstantPool) or something like that instead?

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

Actually I was inspired by getPICLabel in ARMAsmPrinter.cpp but I can change it to 'CP' if it makes more sense.

Yes please. Something that makes the connection with a constant pool clear would be helpful for people to make that connection later on.

labrinea updated this revision to Diff 102530.Jun 14 2017, 5:25 AM

I've renamed the constant pool labels as suggested.

This revision is now accepted and ready to land.Jun 14 2017, 5:29 AM
This revision was automatically updated to reflect the committed changes.
labrinea reopened this revision.Jun 19 2017, 1:17 AM

I've upset a buildbot which runs the address sanitizer:
ERROR: AddressSanitizer:
stack-use-after-scope lib/Target/ARM/ARMISelLowering.cpp:2690
That Twine variable is used illegally. I am reopening the revision.

This revision is now accepted and ready to land.Jun 19 2017, 1:17 AM
labrinea updated this revision to Diff 102994.Jun 19 2017, 1:19 AM

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

christof accepted this revision.Jun 19 2017, 2:11 AM

The Address Sanitizer caught a stack-use-after-scope of a Twine variable. This is now fixed by passing the Twine directly as a function parameter.

That sounds like a good fix to me. Please mention the svn revision number of the original commit you reapply here. The git revision number has not much meaning.

This revision was automatically updated to reflect the committed changes.